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

Properly space and linebreak roles and groups in users table row #2949

Merged
merged 4 commits into from
Jun 7, 2021

Conversation

swaterkamp
Copy link
Member

@swaterkamp swaterkamp commented May 28, 2021

What:
Properly space and linebreak roles and groups in users table row by not using the Divider component, but HorizontalSep with a new wrap prop.

Why:
Adding too many roles and/or groups to a user grew the table row too wide and move parts of it out of screen. It was also not very readable even without growing > 100%/width

How:
Visually inspect how roles and groups are displayed. Add test for wrapped HorizontalSep

Checklist:

  • Tests
  • CHANGELOG Entry
  • Labels for ports to other branches

@swaterkamp swaterkamp added port-to-main Use mergifiy to port PR to master port-to-stable Use mergifiy to port PR to stable port-to-21.10 labels May 28, 2021
@swaterkamp swaterkamp requested a review from saberlynx May 28, 2021 09:17
@swaterkamp swaterkamp self-assigned this May 28, 2021
@swaterkamp swaterkamp requested a review from a team as a code owner May 28, 2021 09:17
@codecov
Copy link

codecov bot commented May 28, 2021

Codecov Report

Merging #2949 (7a5466a) into gsa-20.08 (2f34292) will increase coverage by 0.67%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##           gsa-20.08    #2949      +/-   ##
=============================================
+ Coverage      53.06%   53.74%   +0.67%     
=============================================
  Files           1072     1072              
  Lines          25900    25909       +9     
  Branches        7372     7373       +1     
=============================================
+ Hits           13744    13924     +180     
+ Misses         11034    10881     -153     
+ Partials        1122     1104      -18     
Impacted Files Coverage Δ
gsa/src/web/pages/extras/cvsscalculatorpage.js 94.73% <ø> (ø)
gsa/src/web/pages/extras/feedstatuspage.js 97.67% <ø> (ø)
gsa/src/web/pages/extras/trashcanpage.js 3.27% <ø> (ø)
gsa/src/web/pages/permissions/multipledialog.js 15.78% <ø> (ø)
gsa/src/web/pages/radius/dialog.js 66.66% <ø> (ø)
gsa/src/web/pages/results/details.js 47.12% <ø> (+40.22%) ⬆️
gsa/src/web/pages/start/page.js 51.23% <ø> (ø)
gsa/src/web/pages/users/detailspage.js 33.33% <ø> (ø)
gsa/src/web/pages/users/row.js 26.66% <ø> (ø)
gsa/src/gmp/models/user.js 100.00% <100.00%> (ø)
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6c8b7b...7a5466a. Read the comment docs.

Copy link
Contributor

@bjoernricks bjoernricks left a comment

Choose a reason for hiding this comment

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

Properly space and linebreak roles and groups in users table row by not using the Divider component, which prevents wraps in flexbox. Instead add a ,  to each entry, except the last one, and let flexbox do it's magic (using the Layout component).

Why not using the wrap prop at the divider?

@swaterkamp
Copy link
Member Author

This might be a slightly misleading commit message. The <Divider> is a <Layout> and should be able to handle the wrap, of course. When introducing the comma between the items, however, I used <Layout> instead, because otherwise I would have had to wrap each item and its comma into another span or so, to get the spacing right. Switching to <Layout> kept the component tree a bit simpler and easier to read.

Copy link
Contributor

@bjoernricks bjoernricks left a comment

Choose a reason for hiding this comment

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

For me your solutions looks to complicated. If this can't be fixed with CSS (especially flexbox) alone it should be abstracted into a Divider like component.

@swaterkamp
Copy link
Member Author

Using , is just making this so complicated. I now decided on using a wrappable HorizontalSep, to keep the solution as simple as possible.

@sarahd93 sarahd93 dismissed bjoernricks’s stale review June 7, 2021 14:04

Changes have been implemented

@sarahd93 sarahd93 merged commit 27f0c06 into greenbone:gsa-20.08 Jun 7, 2021
swaterkamp added a commit that referenced this pull request Jun 8, 2021
Properly space and linebreak roles and groups in users table row (backport #2949)
swaterkamp added a commit that referenced this pull request Jun 8, 2021
Properly space and linebreak roles and groups in users table row (backport #2949)
swaterkamp added a commit that referenced this pull request Jun 15, 2021
Properly space and linebreak roles and groups in users table row (backport #2949)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
port-to-main Use mergifiy to port PR to master port-to-stable Use mergifiy to port PR to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants