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

Add CSS listbox size parameters #2

Closed
wants to merge 4 commits into from

Conversation

CodeSandwich
Copy link
Contributor

This PR attempts to make the default CSS more universal with regard to size. It does a few things:

  • Make the listbox size explicitly configurable with custom CSS properties. This includes the width and the height, but also a toggle for forcing the scaling of the listbox to the left. This should make the default CSS easily usable on pages with a wide range of search box locations. The demo page is updated to present this.
  • Remove the @media sections. They were very opinionated, the current approach is more flexible. The viewport width ranges didn't match each other, so there were actually 3 width ranges, each of them with a different set of CSS attributes. The result was that only in the wide viewport the listbox was flexible and good looking, and narrowing it caused glitches.
  • Move .tsmb-form--slash::after sections closer to each other, so it's easier to follow the slash icon behavior
  • Clean up white spaces

Krinkle pushed a commit that referenced this pull request Nov 27, 2023
* Remove distinction between first and second themed row
  on the demo page. Use demo-logo for both instead of omitting the
  image the second time.

* Add right-aligned to the example. This uses hardcoded CSS right
  now, to be improved in #2.

* Fix inconsistent indentation in the demo/index.html file.
@Krinkle
Copy link
Member

Krinkle commented Nov 27, 2023

@CodeSandwich Thanks. I agree with your intent and most of the implementation.

I think the default state regresses a bit as-is, because 60vh feels too narrow in most cases I've seen. And increasing it to 80vh or 100vh doesn't work either because that can cause overflow in most cases, unless the layout has the search widget directly at the edge of the screen.

Previously, we used 100% as width, which makes it adapt to the search widget layout and available width automatically.

Would something like min(30rem, 100%) work instead of min(30rem, 80vh)? Or perhaps all three min(30rem, 80vh, 100%). I think that would mean it effectively starts at 100% on the narrowest screens, then at some point grows upto 80vh even if the search form ends up wider, and is capped at 30rem on typical desktop viewports. In practice, I imagine this would be indistinguishable from leaving out the vh unit, though.

What do you think? I think it'd be great to have the default experience take the available space on mobile (i.e. not significantly less, and also not overflow from the parent 100%).

@Krinkle
Copy link
Member

Krinkle commented Nov 27, 2023

I've committed the clean ups to the demo in 21eaf97 as a start.

@Krinkle Krinkle closed this Nov 27, 2023
@Krinkle Krinkle reopened this Nov 27, 2023
@CodeSandwich
Copy link
Contributor Author

CodeSandwich commented Nov 27, 2023

I agree with all your width concerns, but I think that they are addressed by .tsmb-form [role=listbox] having min-width: 100%;. On narrow screens the 60vh will be overridden by 100% because min-width has higher priority than width. You can try resizing the window displaying the demo, IMO it works well no matter how narrow the window gets.

Would something like min(30rem, 100%) work instead of min(30rem, 80vh)?

This is covered by min-width: 100%;. This property IMO should not be explicitly configurable, and should be hardcoded as much as CSS allows hardcoding. I can't think of a sane use case where anybody would want to make the results dropdown narrower than the search textbox. width is explicitly configurable, so putting min(100%, ... as its default value would be a footgun.

Maybe breaking up --tsmb-size-listbox-width: calc(min(30rem, 60vh)); into --tsmb-size-listbox-width: 30rem; and --tsmb-size-listbox-max-width: 60vh; would be clearer? It would expose another CSS knob to adjust though.

@Krinkle
Copy link
Member

Krinkle commented Nov 28, 2023

@CodeSandwich Thanks for clarifying, I missed the interaction between min-width and width. I made some local changes to that before I did my testing, which I was overly confident in would not affect the outcome. I'll have another look!

@Krinkle
Copy link
Member

Krinkle commented Nov 29, 2023

@CodeSandwich I've narrowed down where it goes wrong in the demo. I modified the patch to limit the margin-left and right: 0 only to demo-right-form, so as to keep the first row as a a themed version of the default left-aligned mode.

In that mode, the listbox starts to overflow the document at certain widths, creating a scrollbar:

Debugging it further, I realize this is specific to the --tsmb-size-listbox-width override. With the default value of min(30rem,60vh) it indeed doesn't cause an overflow in the demo. And more specifically, it is due to the minimum value of 50rem. This makes sense indeed as 30rem is approximately the width of the logo (10rem) + search input (20rem), and when right-aligned, it can go under both. I didn't understand at first where those values came from. It makes sense to me now.

I've updated my branch to move the width and listbox-width override to the demo-right-form class as well.

The remaining question then is:

  • Why 30rem
  • Why 60vh

I believe 30rem, is the closest round number to match the previous default of 500px. Is that right? This is a bit wider than the default input width of 20rem, so there is a chance of overflow there, but seems to work out in practice since 60vh is likely (or guruanteed?) to kick in by that point and trim it down, until eventually it matches the width of the input field (20rem), after which both input and listbox shrink together on narrow viewports.

Does that sound about right?

I've uploaded my amendment at 7b47cbc.

Regarding the new CSS variables. The motivation for the variables is currently to create a stable way to set values that are (or likely will) be used in multiple places, or otherwise need to apply via a rule or selector that isn't selectable through public and stable selectors. For example, the theme colors and font sizes are used in various ways that I think are more future proof this way.

The Styles API shows which selectors are considered "stable", and that includes .tsmb-form [role=listbox]. Can you share a few words around why you'd prefer to set max-height and width via a variable rather than directly? For example, do you foresee re-use of variables on another element or in another calculation at some point?

@CodeSandwich
Copy link
Contributor Author

CodeSandwich commented Nov 29, 2023

Thank you for looking so deeply into it! Before diving deeper into your branch, have you tried this PR in its vanilla form, as in checking out commit ce54138 and seeing if things are working as expected? This could be a good starting point, it would be easier to pinpoint the bugs and the unwanted behaviors, because this commit doesn't have layers of patches in the demo yet.

This is how ce54138 behaves:

Screencast_20231129_121117.mp4

@CodeSandwich
Copy link
Contributor Author

CodeSandwich commented Nov 29, 2023

The Styles API shows which selectors are considered "stable", and that includes .tsmb-form [role=listbox]. Can you share a few words around why you'd prefer to set max-height and width via a variable rather than directly? For example, do you foresee re-use of variables on another element or in another calculation at some point?

Sorry, I haven't noticed the API-Style.md file, I've updated it and pushed to my branch. I would prefer max-height and width as variables to make them stable and explicitly exposed. These are the parameters that the person putting minibar on their page probably should customize, should be able to do that without going too deeply into minibar's default CSS file, and these parameters shouldn't break when upgrading minibar.

This is why I wonder if splitting --tsmb-size-listbox-width into --tsmb-size-listbox-width and --tsmb-size-listbox-max-width is a good idea, this will stabilize 2 variables instead of 1, but OTOH it should be easier to grasp for the dev trying to use minibar without going too deeply into CSS wizardry.

@CodeSandwich
Copy link
Contributor Author

CodeSandwich commented Nov 29, 2023

Could you explain what is your goal of these changes?

I've uploaded my amendment at 7b47cbc.

It seems to avoid the addition of the CSS variable for width, but the demo page seems to be looking good.

@Krinkle
Copy link
Member

Krinkle commented Dec 4, 2023

@CodeSandwich I did test the patch as-is, but it doesn't give me the level of confidence I'd like as the themed mode set both bars to be right-aligned. This left it unclear how the left-aligned case would behave, with uncertainty/ambiguity over the width override (is the value only appropiate for right-aligned? do we require users to calculate a different override for left? or would the default work as-is in a flebox context for left-aligned mode and without overflow bugs?).

Those questions reflect the modifications I made locally.

I would prefer max-height and width as variables […]. These are the parameters that the person putting minibar on their page probably should customize, […]

Thanks for mentioning that. The reason I highlighted (promise stability, reduce selector complexity and/or permit re-use) don't cover this, but I like the idea of recommending them. We could evolve that later into a short-list of recommended styles to consider setting, but that's for later. The reason I'm leaning in that direction is that I don't see a way that these could become anything other than explicitily setting 1 specific CSS propertly on 1 specific stable selector. I don't want to discourage setting individual CSS styles, and would likely decline feature requests that add variables for properties we don't set by default. But for built-in ones, especially if it's likely that each user should think about setting it, I think that makes for a fine addition. Let's roll with it!

@Krinkle Krinkle closed this in ca5519d Dec 4, 2023
@CodeSandwich
Copy link
Contributor Author

CodeSandwich commented Dec 4, 2023

Thanks for the de-facto merge! And a separate thank you for all the effort you put into this PR 🎉 The demo is indeed much better now.

@Krinkle
Copy link
Member

Krinkle commented Dec 4, 2023

@CodeSandwich You're welcome. I want to clarify that your patch wasn't wrong. It just took me a few tries to understand everything that went into it. We could draw the themed/unthemed line differently and also demo/test the custom width behaviour both left and right aligned (as you did originally).

I ended up with a slightly differently opinionated outcome, but this was more the result of me working with it to understand it, than disagreeing on anything in particular. I hope that makes sense!

Krinkle added a commit that referenced this pull request Aug 22, 2024
Follows-up ca5519d, which moved this from applying last,
in a media query, to hide the keyboard short cut on narrow viewports
(likely touchscreen devices); to instead apply before the actual style
unconditionally, thus never actually applying as it is immediately
overriden by another style right below it.

The default for `::after`, is already to not exist / be hidden.

Ref #2.
Krinkle added a commit that referenced this pull request Aug 22, 2024
Follows-up ca5519d, which moved this from applying last,
in a media query, to hide the keyboard short cut on narrow viewports
(likely touchscreen devices); to instead apply before the actual style
unconditionally, thus never actually applying as it is immediately
overriden by another style right below it.

The default for `::after`, is already to not exist / be hidden.

Ref #2.
Closes #4.
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.

2 participants