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

[web] Save space when rendering a selector #898

Merged
merged 4 commits into from
Dec 1, 2023

Conversation

dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Nov 30, 2023

Problem

Back in May, we introduced a preliminary version of a device selector (see #578). Although we like its look and feel, after extending its usage to localization page (#881) and space policy selector (#883), we've realized that there is too much wasted space between selector options. Even after reducing it accidentally as part of #883 (00c36b9, 15aab26)

Solution

Drop the space between options to make the selector looks like what it is: a single widget.

Testing

  • Tested manually

Notes

With that change, it's true that a selector supporting multiple selection could look a bit weird at first sight when there are two or more options selected in a row. That's why I've kept the box-shadow for the selected options, which creates the impression that there is a slight separation between them.

Click to show/hide screenshots
Before After
Screen Shot 2023-11-30 at 21 46 06 Screen Shot 2023-11-30 at 21 48 00

In any case, I'd like to go ahead without adding an exception for the only multiple selection we have at this time. The reason is that we're going to rework these selectors ASAP due to a few known issues. Most probably, the new version will include an extra widget per option: radio for single selection, checkbox for multiple selection. I'm confident on the fact that these widgets will throw away the potential issue we might introduce by removing the wasted space between options.

So, let's save time now 🙏

Screenshots

Before After
Screen Shot 2023-11-30 at 21 44 43 Screen Shot 2023-11-30 at 21 46 51
Click to show/hide more screenshots
Before After
Screen Shot 2023-11-30 at 21 44 49 Screen Shot 2023-11-30 at 21 47 11
Screen Shot 2023-11-30 at 21 44 54 Screen Shot 2023-11-30 at 21 47 21
Screen Shot 2023-11-30 at 21 45 08 Screen Shot 2023-11-30 at 21 47 46
Screen Shot 2023-11-30 at 21 46 21 Screen Shot 2023-11-30 at 21 48 12

@coveralls
Copy link

coveralls commented Nov 30, 2023

Coverage Status

coverage: 75.341%. remained the same
when pulling 8dab1ac on change-selectors-look-and-feel
into bb1c038 on master.

@dgdavid dgdavid force-pushed the change-selectors-look-and-feel branch from 120550c to 8dab1ac Compare November 30, 2023 22:42
@imobachgs imobachgs merged commit 04fa42e into master Dec 1, 2023
@imobachgs imobachgs deleted the change-selectors-look-and-feel branch December 1, 2023 05:54
@dgdavid
Copy link
Contributor Author

dgdavid commented Dec 1, 2023

Thanks for the approval and mege, @imobachgs

@imobachgs imobachgs mentioned this pull request Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants