[WEB-4684] Core navigation component redesign (LeftSidebar, Header, RightSidebar) #2952
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 ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
jamiehenson
force-pushed
the
web-4684-docs-nav-update
branch
from
a0842b5 to
9bb0c18
Compare
jamiehenson
changed the title
jamiehenson
had a problem deploying
to
ably-docs-web-4684-docs-8femuv
| @@ -0,0 +1,156 @@ | |||
| --- | |||
There was a problem hiding this comment.
This is purely to showcase stepped headings. Don't review, it will be binned
jamiehenson
force-pushed
the
web-4684-docs-nav-update
branch
from
0d1606c to
3141514
Compare
| return ( | ||
| <AblyHeader | ||
| nav={ | ||
| <div className="flex items-center justify-between h-16 px-6 fixed w-full z-50 bg-neutral-000 dark:bg-neutral-1300 border-b border-neutral-300 dark:border-neutral-1000"> |
There was a problem hiding this comment.
The inflation in file size here is largely due to abandoning the Header component from ably/ui, this however gives us considerably more control and leads to less complexity in the bigger picture (particularly considering this header looks/feels like a mix between the voltaire header and the dash header). There's more code here, but it's simpler.
| }; | ||
| // Tailwind 'md' breakpoint from tailwind.config.js | ||
| const MD_BREAKPOINT = 1040; | ||
| const CLI_ENABLED = false; |
There was a problem hiding this comment.
Hard-coded for now, because there's no immediate scope for this being a thing
jamiehenson
changed the title
| </button> | ||
| </Tooltip.Trigger> | ||
| <Tooltip.Portal> | ||
| <Tooltip.Content className={tooltipContentClassName}>Open CLI</Tooltip.Content> |
There was a problem hiding this comment.
This is just a question, when using radix I tend to import them like
import {
Tooltip,
TooltipContent,
} from "radix";
Then I use <TooltipContent> what is the difference between not importing and using custom abstraction. or just preference?
There was a problem hiding this comment.
It doesn't matter really, it's just two ways of accessing things off a JS object. You can destructure it like how you're doing it, or import one thing and access things off of it like I'm doing here, the resulting difference is minimal.
I've borrowed this approach from Radix's own documentation, it makes sense here I think because we're accessing a sizeable number of things that are all parts of one larger whole, instead of a small number of entities of varying uses and types.
aralovelace
left a comment
aralovelace
left a comment
There was a problem hiding this comment.
This is such a nice sidebar navigation! very easy to navigate and sleek
One thing I noticed when I go to certain page and select another language in the codebox and refresh its not found
kennethkalmer
left a comment
kennethkalmer
left a comment
There was a problem hiding this comment.
This looks fantastic @jamiehenson 🤩
Some questions inlined, and then this strange thing I was while checking things out in Safari's responsive design mode...
|
Thanks @kennethkalmer / @aralovelace. @aralovelace: I wouldn't worry about language selection for now - that functionality is being reinstated in the UI in the header PR and nothing in this PR should change any of that. @kennethkalmer: that's more of a design limitation but well raised, I'll just hide the tabbed menu visually before it borks |
…lift z-index of top-level accordion items
|
I've addressed the various comments here, and also changed the conditions where left sidebar items are bolded (not on active ancestor, but on the dumber condition of the child accordion being open) - this was done on a hunch and is modifiable in future. |
Also embodies WEB-4685 and WEB-4687. Combining them is suboptimal from a reviewing perspective but they're all kinda interlinked as a review entity.
Description
This PR implements redesigns for the three core navigation components:
LeftSidebar,RightSidebar, andHeader. It's the first wave of changes to come as part of the wider "docs redesign" epic. As such, this doesn't go intomain, butweb-4684-docs-redesign, which will hold this and all related PRs to come.From a technical perspective it's somewhat of a simplification:
LeftSidebarhas broken away from using theably/uiAccordioncomponent in favour of a more targeted usage of Radix Accordion primitives, which simplifies things as we're no longer forcing round pegs into square holes for the sake of component centralisation. This is part of a wider move to separate docs out as its own design system entity.RightSidebarloses the language selector (moved) and the external links (llm links moved, GH links removed)Headeralso moves away fromably/uilessening a dependency onHeaderand also using Radix primitives for greater control.Testing
https://ably-docs-web-4684-docs-8femuv.herokuapp.com/docs/
Acceptance criteria: looks and acts like the Figma.
Expectations: this review is mostly about the technicals - the whole picture will be presented to devex/design when all is ready. Given that there are many parts to this overall work and a merge here won't go to prod, it's more about ensuring that the bones are good rather than it being a 100% ready-to-go piece of work. That said, there's nothing I'm knowingly leaving out here.
Resources: