Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix ip address ui in host details #1944

Merged
merged 20 commits into from
Oct 27, 2023
Merged

Fix ip address ui in host details #1944

merged 20 commits into from
Oct 27, 2023

Conversation

EMaksy
Copy link
Member

@EMaksy EMaksy commented Oct 20, 2023 •
edited
Loading

Description

This pr will fix IP address overflow in Host Details view.
Adding additionally a tooltip when there are more then 2 ip addresses. This has the nice effect that the user still can have an overview of all ip's

Problem:

The Host Details view didn't handle multiple IP addresses, leading to an overflow issue.

image

Solution:

Host Details view can now manage and display many IP addresses without breaking the ui

10 IP's
image
5 IP's
image
3 IP's
image
2 IP's
image

How was this tested?

Added automated test

@EMaksy EMaksy added bug Something isn't working enhancement New feature or request javascript Pull requests that update Javascript code labels Oct 20, 2023
@EMaksy EMaksy self-assigned this Oct 20, 2023
@EMaksy EMaksy force-pushed the fix_ip_adress_in_host_details branch 4 times, most recently from 91d12b8 to dd5d048 Compare October 23, 2023 15:12
@EMaksy EMaksy added the env Create an ephimeral environment for the pr branch label Oct 23, 2023
@EMaksy EMaksy force-pushed the fix_ip_adress_in_host_details branch 2 times, most recently from 81dbbcd to 8e8d860 Compare October 23, 2023 15:23
@EMaksy EMaksy marked this pull request as ready for review October 24, 2023 07:10
arbulu89
arbulu89 requested changes Oct 24, 2023
View reviewed changes
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @EMaksy
Some comments/suggestions

jagabomb
jagabomb reviewed Oct 24, 2023
View reviewed changes
Copy link
Contributor

@jagabomb jagabomb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EMaksy So far so good, can I be annoying and ask you to add a margin left of the start of the IP addresses (e.g: 100.180.100.110...), whenever the icon is displayed? I like how the icon has a gap between the text in the Saptune Tuning section. I have attached an example below.
Screenshot 2023-10-24 at 10 49 48

@EMaksy EMaksy force-pushed the fix_ip_adress_in_host_details branch 2 times, most recently from 994068b to fbc903e Compare October 24, 2023 12:39
@EMaksy EMaksy requested a review from dottorblaster October 24, 2023 12:40
@EMaksy EMaksy force-pushed the fix_ip_adress_in_host_details branch from fbc903e to 0e1da3f Compare October 24, 2023 12:49
@arbulu89 arbulu89 changed the title Fix ip adress ui in host details Fix ip address ui in host details Oct 25, 2023
@EMaksy EMaksy force-pushed the fix_ip_adress_in_host_details branch 7 times, most recently from 274ac43 to 5a44f47 Compare October 25, 2023 10:28
jagabomb
jagabomb approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@jagabomb jagabomb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Small thing is should we write the title with a capital "A' so that it is IP Addresses. It is the only parameter in sentence case currently in this section. Other than that I able happy.

arbulu89
arbulu89 requested changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EMaksy
Almost there!
Let's put the story in the already existing HostDetails view storybook

dottorblaster
dottorblaster reviewed Oct 26, 2023
View reviewed changes
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment

@EMaksy EMaksy force-pushed the fix_ip_adress_in_host_details branch 2 times, most recently from 10997a1 to 93177a4 Compare October 26, 2023 11:29
arbulu89
arbulu89 approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @EMaksy

rtorrero
rtorrero approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@rtorrero rtorrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @EMaksy nothing to add!

@EMaksy EMaksy force-pushed the fix_ip_adress_in_host_details branch from 93177a4 to 7fedd0d Compare October 27, 2023 07:56
EMaksy and others added 20 commits October 27, 2023 09:57
@EMaksy
Move host details cluster agent ip summary to extra component
988f7da
@EMaksy
Improve customizability of ListView and Tooltip component
a99d325
@EMaksy
Refactor tooltip content renderer
3c6c3e0
@EMaksy
Add test wip for new cluster agent ip summary component wip
a25ba63
@EMaksy
Change when the tooltip icon should appear from 3 to 2 and add spacin…
32de520 
…g between ip's
@EMaksy
Remove added tooltip childrenClassName
5ddfe60
@EMaksy
Sort HostClusterAgentIpSummary props by alphabet
1172044
@EMaksy
Update fixture for the expected ip's
56e4439
@EMaksy
Rename component to HostSummary, fix typos and rename variables
0ef0353
@EMaksy
Add spacing between icon and ip's
c8d2c8a
@EMaksy
Change the tooltip content formatting from flex to grid
c858af3
@EMaksy
Refactor ListView by adding a new rowClassName to improve customizabi…
bab52e5 
…lity
@EMaksy
Remove unused css classnames and fix typos
b54e0b0
@EMaksy
Add storybook story for hostSummary
b52c5a8
@EMaksy
Improve test description
0e4f044
@EMaksy
change addresses to Addresses in title
602f972
@EMaksy
Rename cellClassName to contentClassName
03520c1
@EMaksy
Reorder import by external/internal and alphabet
1eb97a6
@EMaksy
Fix cypress test for the changed ip address title
7380a26
@EMaksy
Remove unrequited story and enrich existing story
7fedd0d
@EMaksy EMaksy merged commit 7ea258a into main Oct 27, 2023
@EMaksy EMaksy deleted the fix_ip_adress_in_host_details branch October 27, 2023 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dottorblaster dottorblaster dottorblaster left review comments

@rtorrero rtorrero rtorrero approved these changes

@arbulu89 arbulu89 arbulu89 approved these changes

@jagabomb jagabomb jagabomb approved these changes

@CDimonaco CDimonaco Awaiting requested review from CDimonaco

Assignees

@EMaksy EMaksy

Labels
bug Something isn't working enhancement New feature or request env Create an ephimeral environment for the pr branch javascript Pull requests that update Javascript code
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@EMaksy @dottorblaster @rtorrero @arbulu89 @jagabomb