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.

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

ui: Fix spacing issues in composite row detail section #9930

Closed
wants to merge 6 commits into from

Conversation

kaxcode
Copy link
Contributor

@kaxcode kaxcode commented Mar 25, 2021

🐛 Fix-ups in all the lists using the %composite-row styling component.

The following fixes were done:

  • Replaced definition lists with spans.

NSpace List Before:
npsace_list_before

NSpace List After:
nspace_list_after

  • Show CopyButton if it's the first item in details

Lock Session List Before:
lock_session_before

Lock Session List After:
lock_session_after

  • Remove extra spacing from CopyButton

Upstream List Before:
upstream_list_before

Upstream List After:
upstream_list_after

  • Added left padding to the details of lists with health check icon in the header

Node List Before:
node_list_before

Node List After:

node_list_after

  • Move copy button to be in span tag

@kaxcode kaxcode added theme/ui Anything related to the UI pr/no-changelog PR does not need a corresponding .changelog entry labels Mar 25, 2021
@kaxcode kaxcode requested a review from johncowen March 25, 2021 20:35
@kaxcode kaxcode force-pushed the ui/bug/spacing-composite-row branch from cac6486 to 5a61dd9 Compare March 25, 2021 20:39
@vercel vercel bot temporarily deployed to Preview – consul March 25, 2021 20:39 Inactive
Copy link
Contributor

@johncowen johncowen left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look at this!

Could you screengrab some "before and afters" for me, I'm just looking at the issue for this and it says it's about equalling out the spacing between groups of Policies/Roles with the individual Policies/Roles themselves in for example Namespaces (so making the space between the groups the same as the space between the things in the groups)

There seems to be a lot of change here for a spacing tweak and I'm not sure what else has changed visually. Maybe you didn't mean to commit all of this or is it a "feed two birds with one scone" thing? For example I saw a bunch of conditionals in the session listing.

Move copy button to be in dt tag.

We've been trying not to put CopyButtons into dt tags and we've been specifically moving CopyButtons out of them as something with no text doesn't make for a great title from a semantic and/or accessibility viewpoint., for example here #8850 .

Anyway let me know and I can take another pass at it.

@kaxcode
Copy link
Contributor Author

kaxcode commented Apr 19, 2021

We've been trying not to put CopyButtons into dt tags and we've been specifically moving CopyButtons out of them as something with no text doesn't make for a great title from a semantic and/or accessibility viewpoint., for example here #8850 .

Oh, I see. Using a dl doesn't make sense with a copy button in these composite rows. I will be putting the copy buttons in a span if they are in a composite row.

@kaxcode
Copy link
Contributor Author

kaxcode commented Apr 19, 2021

Could you screengrab some "before and afters" for me, I'm just looking at the issue for this and it says it's about equalling out the spacing between groups of Policies/Roles with the individual Policies/Roles themselves in for example Namespaces (so making the space between the groups the same as the space between the things in the groups)

I've added some screenshots. Yes, I did fix all the nspace list issues.

There seems to be a lot of change here for a spacing tweak and I'm not sure what else has changed visually. Maybe you didn't mean to commit all of this or is it a "feed two birds with one scone" thing? For example I saw a bunch of conditionals in the session listing.

Not every change was a bug. Some of it was just refactoring such as removing classes unused, removing unnecessary dl tags, and other spacing requests.

@kaxcode
Copy link
Contributor Author

kaxcode commented Apr 19, 2021

@johncowen I've added some screenshots and provided examples to the changes made.

@kaxcode
Copy link
Contributor Author

kaxcode commented Apr 19, 2021

This PR has been out for a bit. Rebased against master.

@johncowen
Copy link
Contributor

Great! Thanks for the screengrabs!

Could we split out the changes for hiding certain Session listing icons when the values aren't set to a separate PR, those changes feel the most like they could possibly use a separate PR.

Lemme know and I can take another look.

@kaxcode
Copy link
Contributor Author

kaxcode commented Apr 20, 2021

Great! Thanks for the screengrabs!

Could we split out the changes for hiding certain Session listing icons when the values aren't set to a separate PR, those changes feel the most like they could possibly use a separate PR.

Lemme know and I can take another look.

@johncowen it would be hard to split that out at this point. Also, not having those conditionals creates spacing issues which is what this PR is about.

@vercel vercel bot temporarily deployed to Preview – consul April 22, 2021 14:45 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 22, 2021 14:45 Inactive
@kaxcode kaxcode removed the pr/no-changelog PR does not need a corresponding .changelog entry label Apr 22, 2021
@kaxcode
Copy link
Contributor Author

kaxcode commented Apr 22, 2021

@johncowen removed the conditionals from Lock Sessions and I've added a changelog.

@johncowen
Copy link
Contributor

Cool thanks for doing that! I think I have one more question - for the upstream list should the detail 'line/row' go under the healthcheck icon, or over to the right of it? I'm actually super glad you added those grabs as I think I've seen another little bug we could fix up.

@kaxcode
Copy link
Contributor Author

kaxcode commented Apr 26, 2021

Cool thanks for doing that! I think I have one more question - for the upstream list should the detail 'line/row' go under the healthcheck icon, or over to the right of it? I'm actually super glad you added those grabs as I think I've seen another little bug we could fix up.

@johncowen I've added the spacing for upstreams list in a separate commit.

@johncowen
Copy link
Contributor

Hey

I've just noticed that sometimes the rows in the Service > Upstreams listing have healthcheck icons and sometimes they don't, so applying the fix here makes the upstreams listing look like the how the nspace listing previously was:

Screenshot 2021-04-29 at 14 18 40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants