[WEB-4242] Mobile navigation fixes #2486
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant LS as LeftSidebar
participant DOM as DOM/Accordion
U->>LS: Click a product nav entry
LS->>LS: Determine target accordion (left-nav/mobile-nav)
LS->>LS: Wait briefly using setTimeout
LS->>DOM: Scroll to open accordion element
sequenceDiagram
participant U as User
participant LS as LeftSidebar
participant Router as Router
U->>LS: Click "Home" nav entry
LS->>Router: Trigger navigate('/docs')
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jamiehenson
force-pushed
the
web-4242-docs-mobile-tweaks
branch
from
5ad391c to
6b28e72
Compare
jamiehenson
force-pushed
the
web-4242-docs-mobile-tweaks
branch
from
6b28e72 to
8f851ab
Compare
| @@ -33,30 +34,6 @@ const Header: React.FC<HeaderProps> = ({ searchBar = true }) => { | |||
| // } | |||
| // }; | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
This now lives in ably-ui and was left behind after the back-merge
| @@ -86,7 +63,7 @@ const Header: React.FC<HeaderProps> = ({ searchBar = true }) => { | |||
| mobileNav={<LeftSidebar inHeader key="nav-mobile-documentation-tab" />} | |||
| searchButton={ | |||
| <button | |||
| className="cursor-pointer focus-base rounded" | |||
| className="cursor-pointer focus-base rounded px-0 pt-4 text-neutral-1300 dark:text-neutral-000" | |||
There was a problem hiding this comment.
🍎 overrides button colouring with its own blue, how thoughtful
| @@ -125,6 +102,7 @@ const Header: React.FC<HeaderProps> = ({ searchBar = true }) => { | |||
| ]} | |||
| sessionState={sessionState} | |||
| logoHref="/docs" | |||
| location={location} | |||
There was a problem hiding this comment.
Piping this in instructs the header menu to close when it changes.
| @@ -130,6 +130,26 @@ const constructProductNavData = ( | |||
| return { | |||
| name: product.name, | |||
| icon: activePageTree[0]?.page.name === product.name ? product.icon.open : product.icon.closed, | |||
| onClick: () => { | |||
There was a problem hiding this comment.
When a top-level accordion item is clicked, then it tries to bring the header row into view if it isn't visible - this happens after the opening animation has concluded, to ensure that the scrollTo has the right point of reference.
| * @param {...number} heights - An array of heights in pixels. | ||
| * @returns {string} The CSS calc expression for the maximum height. | ||
| */ | ||
| export const componentMaxHeight = (...heights: number[]): string => { |
There was a problem hiding this comment.
This lives in ably-ui now, along with the HEADER_* values
| @@ -29,7 +29,7 @@ export const CodeBlock: FC<{ children: React.ReactNode; language: string }> = ({ | |||
| }; | |||
|
|
|||
| return ( | |||
| <pre className="bg-cool-black text-white p-0 rounded-lg relative"> | |||
| <pre className="bg-cool-black text-white p-0 rounded-lg relative max-w-[calc(100vw-48px)]"> | |||
There was a problem hiding this comment.
@kennethkalmer this is the thing that caps the page width
There was a problem hiding this comment.
@jamiehenson ooi would this need dvw instead of vw, or am I overeager to apply my new knowledge? 😃
There was a problem hiding this comment.
It's nifty isn't it. Such a clean solution. There's less of a need for it for width I would imagine (less funny business going on horizontally, only scrollbars really), but for height, yes. I think the best foot forward would be to lead with dvh and offer an equivalent vh class as well as a fallback - to appease our Firefox user
There was a problem hiding this comment.
I've included a fallback for the 100vh helper in ably-ui, but not doing anything here with the 100vws
src/components/Layout/utils/nav.ts
Outdated
| @@ -131,7 +131,7 @@ export const commonAccordionOptions = ( | |||
| { | |||
| 'my-12': topLevel && inHeader, | |||
| 'h-40 ui-text-menu1 !font-bold md:ui-text-menu4 px-16': topLevel, | |||
| 'h-[1rem] ui-text-menu2 !font-semibold md:ui-text-menu4': !topLevel, | |||
| 'h-[1.625em] md:h-[1.375em] ui-text-menu2 !font-semibold md:ui-text-menu4': !topLevel, | |||
There was a problem hiding this comment.
Make the height respect the line-height properties of the surrounding text
jamiehenson
force-pushed
the
web-4242-docs-mobile-tweaks
branch
from
8f851ab to
abffc0a
Compare
jamiehenson
force-pushed
the
web-4242-docs-mobile-tweaks
branch
from
abffc0a to
2c17ca5
Compare
jamiehenson
force-pushed
the
web-4242-docs-mobile-tweaks
branch
from
2c17ca5 to
3371098
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Markdown/CodeBlock.tsx (1)
32-32: Consider adding responsive breakpoint for consistencyThe
max-w-[calc(100vw-48px)]constraint is added to prevent overflow on mobile, but unlike the<dl>component, there's no responsive class to restore full width on larger screens (e.g.,sm:max-w-full). Consider adding this for consistency with other components, unless the capped width is intentional at all screen sizes.- <pre className="bg-cool-black text-white p-0 rounded-lg relative max-w-[calc(100vw-48px)]"> + <pre className="bg-cool-black text-white p-0 rounded-lg relative max-w-[calc(100vw-48px)] sm:max-w-full">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/components/blocks/software/__snapshots__/Pre.test.tsx.snapis excluded by!**/*.snapsrc/components/blocks/wrappers/__snapshots__/ConditionalChildrenLanguageDisplay.test.js.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
package.json(1 hunks)src/components/Layout/Header.test.tsx(2 hunks)src/components/Layout/Header.tsx(3 hunks)src/components/Layout/LanguageSelector.tsx(1 hunks)src/components/Layout/Layout.tsx(1 hunks)src/components/Layout/LeftSidebar.tsx(4 hunks)src/components/Layout/RightSidebar.tsx(1 hunks)src/components/Layout/utils/heights.ts(1 hunks)src/components/Layout/utils/nav.ts(2 hunks)src/components/Markdown/CodeBlock.tsx(1 hunks)src/components/Redoc/Loader.tsx(1 hunks)src/components/blocks/list/Dl/index.tsx(1 hunks)src/components/blocks/software/Pre.tsx(2 hunks)src/components/blocks/table/Table.tsx(1 hunks)
🔇 Additional comments (20)
package.json (1)
42-42: Remember to update the development dependency before mergingThis development version (
15.8.1-dev.1a27ad3f) of the@ably/uipackage should be replaced with a stable version before merging, as noted in your previous comment.src/components/Layout/Layout.tsx (1)
27-27: Good responsive design improvementThe change from a fixed gap to responsive gap sizes (
md:gap-48,lg:gap-64,xl:gap-80) improves the mobile experience by providing appropriate spacing at different viewport sizes.src/components/blocks/list/Dl/index.tsx (1)
9-9: Appropriate mobile width constraint addedAdding
max-w-[calc(100vw-48px)] sm:max-w-fullprevents content overflow on small screens while maintaining full width on larger screens. This is a good responsive design pattern.src/components/blocks/table/Table.tsx (1)
15-15: Good enhancement for responsive design.Adding the responsive width constraints ensures that tables display properly on mobile devices without overflowing, while still using the full available width on larger screens.
src/components/Redoc/Loader.tsx (1)
7-7: Good practice to pin the Redoc version to a specific release.Changing from
latestto a specific version (v2.4.0) improves stability by:
- Preventing unexpected breaking changes from newer versions
- Ensuring consistent behavior across all environments
- Allowing controlled dependency upgrades
src/components/Layout/LanguageSelector.tsx (1)
8-8: Good refactoring of height constants to a centralized location.Moving common height constants (
componentMaxHeight,HEADER_BOTTOM_MARGIN,HEADER_HEIGHT) to the shared@ably/uipackage while keeping app-specific constants local is a good architectural decision that promotes reusability and maintainability.Also applies to: 14-14
src/components/Layout/utils/heights.ts (1)
6-6: Appropriate cleanup after constants migration.The constants have been properly moved to
@ably/uias mentioned in previous feedback, while keeping app-specific constants in this file.src/components/Layout/Header.tsx (2)
1-1: LGTM: Good refactoring to use location prop for menu stateThe removal of local state management in favor of passing the location prop to AblyHeader is a cleaner approach. This matches the implementation in the ably-ui repository and ensures the mobile menu closes when navigation occurs.
Also applies to: 7-7, 14-14, 105-105
66-66: Improved button styling for better iOS compatibilityThe updated classname addresses the iOS-specific styling issue mentioned in the PR description.
src/components/blocks/software/Pre.tsx (1)
67-68: Good refactoring: Extracted duplicated class names into a constantExtracting the duplicate class name string into a
codeClassNameconstant improves maintainability and ensures consistency across multiple usage points. This is a good application of the DRY principle.Also applies to: 74-74, 84-84
src/components/Layout/RightSidebar.tsx (1)
6-6: Improved organization: Centralized height constantsGood refactoring to source common height constants from the central
@ably/uipackage rather than local modules. This promotes consistency across the codebase and makes future height-related changes easier to manage.Also applies to: 12-12
src/components/Layout/utils/nav.ts (2)
3-3: Centralized height constants importGood update to import height constants from the shared
@ably/uipackage, maintaining consistency with the rest of the codebase.
134-134: Improved responsive styling with dynamic heightThe change from fixed height to minimum height using
emunits will ensure that the accordion header respects the line-height properties of the surrounding text, fixing the mobile responsiveness issue mentioned in the PR.src/components/Layout/Header.test.tsx (3)
32-34: Appropriate mock for location-based navigationThe addition of the
@reach/routermock provides the necessaryuseLocationfunction for testing, which is a good practice since it allows for testing location-dependent behavior in the Header component.
36-38: Good test isolation with LanguageSelector mockMocking the LanguageSelector component properly isolates the Header tests from implementation details of the language selection functionality.
103-107: Enhanced test coverage with expanded account objectThe expanded account object structure with id, name, and links properties provides more comprehensive coverage of the user context state that the Header component likely consumes.
src/components/Layout/LeftSidebar.tsx (4)
2-2: Correctly updated imports for navigationThe addition of
navigateanduseLocationimports supports the new navigation capabilities in the component.
133-152: Useful accordion scrolling for improved UXThe onClick handler improves navigation usability by automatically scrolling to open accordion items after clicking. This is especially helpful on mobile devices.
186-196: Good addition of Home navigation optionAdding a Home entry to the navigation when in header mode provides users with a clear way to return to the documentation landing page.
227-238: Simplified component structureThe refactored Accordion rendering creates a more maintainable and cleaner component structure by dynamically determining the ID based on context rather than using conditional rendering.
jamiehenson
force-pushed
the
web-4242-docs-mobile-tweaks
branch
from
3371098 to
b4db9b4
Compare
jamiehenson
force-pushed
the
web-4242-docs-mobile-tweaks
branch
from
b4db9b4 to
5f381ed
Compare
jamiehenson
force-pushed
the
web-4242-docs-mobile-tweaks
branch
from
bc69064 to
971fd52
Compare
jamiehenson
force-pushed
the
web-4242-docs-mobile-tweaks
branch
from
971fd52 to
58b4d5a
Compare
jamiehenson
force-pushed
the
web-4242-docs-mobile-tweaks
branch
from
58b4d5a to
54dcb88
Compare
Addresses all the items on this ticket, as well as this support thread, and iOS compatibility feedback passed to @aralovelace.
Requires ably/ably-ui#703. This won't merge until that package is available and introduced here.
Review link: https://ably-docs-web-4242-docs-gg1ahr.herokuapp.com/
Summary by CodeRabbit
Chores
User Interface Enhancements