StackExchange / Stacks

Stack Overflow’s Design System
https://stackoverflow.design
MIT License
610 stars 90 forks source link

fix(topbar): ensure search renders correctly at small widths #1693

Closed dancormier closed 6 months ago

dancormier commented 6 months ago

Addresses https://meta.stackexchange.com/questions/398680/too-small-search-bar See also https://stackexchange.slack.com/archives/C27RWNQN9/p1711421438736069?thread_ts=1711408665.906589&cid=C27RWNQN9

Note: Core currently has a hotfix that should be removed once it consumes a Stacks Classic version that includes this fix.


The issue

This PR fixes responsive topbar styles, specifically styles on the searchbar. In Core, responsive topbar styles were rendering as:

@media (max-width: 640px) {
  html.html__responsive:not(.html__unpinned-leftnav) .s-topbar html.html__responsive:not(.html__unpinned-leftnav) .s-topbar--searchbar {
    --_tb-searchbar-d: none;
    --_tb-searchbar-p: var(--su8) var(--su12);
    --_tb-searchbar-open-d: flex;
    --_tb-searchbar-open-mxw: none;
    background: var(--theme-topbar-item-background-hover, var(--black-200));
    left: 0;
    max-width: 100%;
    position: absolute;
    right: 0;
    top: 100%;
  }
  html.html__responsive:not(.html__unpinned-leftnav) .s-topbar html.html__responsive:not(.html__unpinned-leftnav) .s-topbar--searchbar .s-select {
    width: 25% !important;
  }
}
@media (max-width: 640px) {
  html.html__responsive.html__unpinned-leftnav .s-topbar html.html__responsive.html__unpinned-leftnav .s-topbar--searchbar {
    --_tb-searchbar-d: none;
    --_tb-searchbar-p: var(--su8) var(--su12);
    --_tb-searchbar-open-d: flex;
    --_tb-searchbar-open-mxw: none;
    background: var(--theme-topbar-item-background-hover, var(--black-200));
    left: 0;
    max-width: 100%;
    position: absolute;
    right: 0;
    top: 100%;
  }
  html.html__responsive.html__unpinned-leftnav .s-topbar html.html__responsive.html__unpinned-leftnav .s-topbar--searchbar .s-select {
    width: 25% !important;
  }
}

Note the extra html.html__responsive.html__unpinned-leftnav. I believe this is related to the @force-selector code in the responsive less functions, but I haven't totally deciphered why the CSS renders differently in consumers. I'll make a ticket to understand and (possibly) refactor our responsive helpers so they're easier to use and understand.

The fix

A stable solution seems to be to remove any nested selectors in the responsive functions, instead opting to include the responsive functions within the nested selectors themselves. I've tested this in Core and it resolves the issue.

FYI you should see no visible change in the Stacks docs.


To test

In Stacks

  1. Go to the Stacks docs Topbar page.
  2. Resize your browser window so it's smaller than 640px width
    • See the search input be replaced with a button containing the search icon
  3. Click the button with the search icon
    • See the search input show below the topbar
  4. Resize your browser window so it's larger than 640px width
    • See the search button hide and the search input show in the topbar again

In Core

[!NOTE] You should be able to use npm link to check instead of the instructions below.

  1. Copy the entire contents of the Stacks topbar.less file
  2. Replace the contents of StackOverflow/node_modules/@stackoverflow/stacks/lib/components/topbar/topbar.less with the copied code
  3. Run npm run build in the StackOverflow directory
  4. View the topbar in your local Core instance, repeating steps 2-4 in the "In Stacks" section above

@CGuindon for visibility.

netlify[bot] commented 6 months ago

Deploy Preview for stacks ready!

Name Link
Latest commit 14d2524e2257bfd540e26ece2758a1d120218af6
Latest deploy log https://app.netlify.com/sites/stacks/deploys/6602ff445136470008bd095f
Deploy Preview https://deploy-preview-1693--stacks.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dancormier commented 6 months ago

Looks fine in Stacks docs but I guess we can't test the additional popup core has when you click on the search bar until it's merged in. Maybe we don't need to?

@CGuindon I've tested Core by manually replacing the topbar styles (see below). It's a little bit of a pain to check this, but we can make sure it works well in Core once we cut a new Stacks Classic release and update Core with it.

20240326-015401

dancormier commented 6 months ago

@giamir I'm going to merge this so I can replace the hotfix I added to Core with a new Stacks Classic patch version. Feel free to review when you have a moment. Thanks in advance