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

refactor(theme-common): allow optional desktopBreakpoint param in useWindowSize #9335

Merged
merged 16 commits into from
Dec 1, 2023

Conversation

jgarrow
Copy link
Contributor

@jgarrow jgarrow commented Sep 21, 2023

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

The desktopThresholdWidth in the useWindowSize hook is currently hardcoded at 996px. This makes it impossible to customize the breakpoint for the navbar, which creates inconsistencies when other CSS media query breakpoints are customized for the rest of the site.

This change allows for customization of that value by allowing for an optional desktopThresholdWidth parameter to be passed to the useWindowSize hook. If none is provided, the original 996px value is used.

Test Plan

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

Discussed previously in #9261, decided on a simpler implementation with a smaller footprint.

@netlify
Copy link

netlify bot commented Sep 21, 2023

[V2]

Name Link
🔨 Latest commit c063b18
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/656a04072e8e1700080f38b9
😎 Deploy Preview https://deploy-preview-9335--docusaurus-2.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.

@github-actions
Copy link

github-actions bot commented Sep 21, 2023

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 79 🟢 98 🟢 100 🟢 100 🟠 89 Report
/docs/installation 🟠 88 🟢 98 🟢 100 🟢 100 🟠 89 Report
/docs/category/getting-started 🟠 74 🟢 100 🟢 100 🟢 90 🟠 89 Report
/blog 🟠 74 🟢 100 🟢 100 🟢 90 🟠 89 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 63 🟢 97 🟢 100 🟢 100 🟠 89 Report
/blog/tags/release 🟠 73 🟢 100 🟢 100 🟠 80 🟠 89 Report
/blog/tags 🟠 77 🟢 100 🟢 100 🟢 90 🟠 89 Report

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

One design suggestion. Otherwise I'm +1 on this. We would preferably also add some docs to the Swizzling page, explaining that you would need to keep this value in sync with custom CSS.

@@ -43,17 +46,17 @@ const DevSimulateSSR = process.env.NODE_ENV === 'development' && true;
* In development mode, this hook will still return `"ssr"` for one second, to
* catch potential layout shifts, similar to strict mode calling effects twice.
*/
export function useWindowSize(): WindowSize {
export function useWindowSize(desktopThresholdWidth = 996): WindowSize {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not fan of using a single arg there, please use a flexible object instead because we don't want to do a breaking change if we later introduce a new breakpoint some day or if you or someone else come with the need for 3 different sizes. If we add flexibility for your use case, let's make it flexible enough for other use cases as well.

This lib has a decent API to copy imho: https://github.com/iiroj/use-breakpoint

const BREAKPOINTS = { mobile: 0, tablet: 768, desktop: 1280 }

const CurrentBreakpoint = () => {
  const { breakpoint, maxWidth, minWidth } = useBreakpoint(
    BREAKPOINTS,
    'desktop'
  )
  return <p>The current breakpoint is {breakpoint}!</p>
}

(by the way you could as well use this lib directly in your code, similar behavior to what we implemented here).

Comment on lines 183 to 187
:::tip Customizing the breakpoint

Some React components, such as the header and the sidebar, implement different JavaScript logic when in mobile view by calling the [`useWindowSize`](./swizzling.mdx) hook. If you change the breakpoint value in your custom CSS, you probably also want to update the invocations of the `useWindowSize` hook by swizzling the components it's used in and passing an explicit option argument. For example, to change the breakpoint to 796px, pass the argument `796` to the `useWindowSize` hook.

:::
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Josh-Cena to be honest I'm not sure we want to document this

@theme-common has many APIs and we only document a few ones that are meant to be used in userland explicitly. Even if this is considered "public API" and we want to avoid breaking changes on it, it doesn't mean we should document it IMHO.

If power users want to customize breakpoints and retro-engineer our helpers and want to use it, fine (although as I said you can provide your own code as well). But do we want to encourage usage, I'm not sure at all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we mention customizing the CSS break point, we definitely need to mention customizing the JS break point. Users creating custom components (maybe through swizzling) always need to know both.

@Josh-Cena Josh-Cena changed the title feat(theme-common): Allow optional desktopThresholdWidth param in useWindowSize feat(theme-common): allow optional desktopThresholdWidth param in useWindowSize Oct 5, 2023
@neoakris
Copy link

neoakris commented Nov 20, 2023

Commenting to add myself as follower.
I too wish this was configurable as I wish I could allow mobile users in landscape mode to use the desktop UX, which is significantly better IMO.
(ATM even if they click show desktop view of the site, that right menu ToC gets hidden due to the hard coded value of 996px.)
And to make matters worse:

  • My phone has 1440 x 3040 physical pixel density. But thanks to device-independent pixels. Unfortunately, once the physical pixels get converted to virtual pixels, the 996px threshold is no longer met. (Unless I use developer mode hacks that can only fix it for my phone, and that end users won't know how to do.)
  • I also learned by using a developer mode workaround to adjust minimum width that the desktop version of the site (specifically the TOC on the right side for docs) looks and works great on a phone, it just refuses to display by default due to the hard coded threshold.

Side Note: I briefly played around with swizzling, but couldn't find a way to prevent mobile view from hiding the right side ToC of docs. I'm guessing this MR would make it easier to prevent the mobile view from taking effect (personally I wish mobile view could be turned off, as I think it'd improve consistency.)

@hammerofthor8 of the previous PR also seemed interested in it.

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Nov 23, 2023

What matters slightly more is the physical width, not the pixel width—the former is what the user actually perceives. Even for a phone screen in landscape, the screen is not wide enough unless you have a phone that's 20cm long. Therefore I suspect if you use the desktop layout the items are going to either be too cramped or too small. I could be wrong, though, and I'm happy to see what it actually looks like on a phone.

@neoakris
Copy link

neoakris commented Nov 23, 2023

@Josh-Cena docusaurus's desktop view looks great on mobile! / far better than the Mobile UX in my opinion.
The phone has more than enough resolution for desktop view to work.

Here's some screenshots of a Work in Progress Website from My Phone:

  • The fact that the left side bar menu is collapsible is all that's needed to support small screens.

  • Screenshot_20231123_144532_Chrome_Desktop_view_sidebar_expanded

  • Screenshot_20231123_145140_Chrome_Desktop_view_sidebar_collapsed

  • And below is the terrible (by comparison) default Mobile UX

  • Notice a key detail (of why I say the mobile UX is terrible):
    The amount of text seen in the Mobile UX is exactly the same! as the 1st screenshot / Desktop View (with the left side bar, and right ToC enabled.) (It's just slightly shrunken down, but very readable) (the non shrunken one feels over zoomed to me, a user could always pinch zoom in to that level of detail if needed.)

  • Screenshot_20231123_145708_Firefox_Mobile_view

  • The one place I think the current Mobile UX is good/makes sense is portrait mode. (In portrait mode the desktop view is too tiny, and the mobile view is legitimately better.)

  • Actually now that I think about it what'd be ideal is if a configuration option existed to make it so:

    • Landscape view on mobile phone always used the "Desktop View"
    • Then portrait mode on mobile phone & really narrow width, switched to the current "Mobile View"

Btw here's a screenshot I've been tentatively planning to add to my site:
(I haven't QA'd it against spelling / grammar yet and it's only hosted on localhost atm.)
image

@Josh-Cena
Copy link
Collaborator

Our biggest worry is that the navbar doesn't look good, as your example has already shown. We purposedly set the breakpoint to a very large number (most style frameworks use 768px instead), because for typical numbers of navbar items, the items would already get wrapped below 1000px and it's not easy to lay them out nicely.

@neoakris
Copy link

neoakris commented Nov 24, 2023

Josh, Thanks for offering background contextual explanation, and reading about my use case that aligns with the OP's.

I think I get your reasoning, and that'd be solid reason to keep the defaults as they currently are. (I was playing around with CSS overrides earlier today I saw an interesting text overlap scenario that made me think that's why I saw a lot of spare white space in some places, so I think I have some understanding of what you mean.)
That said if possible I'd love to have this be a configurable value / have the ability to override the default for my use case.

(This merge requests seems to be helping to work towards making that a configurable value, and in case of the rough spots I think you're pointing out (menu text wrapping / not easy to layout nicely), I'd be able to do things like cut down on menu items, shorten menu names to single words, replace menu text with images, and manually QA edge case UI oddities.)

@Josh-Cena
Copy link
Collaborator

I definitely have no intention to prevent this from being merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR has been backported to a stable version of Docusaurus CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants