-
Notifications
You must be signed in to change notification settings - Fork 0
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 Developers and About menus to site nav #585
Conversation
✅ Deploy Preview for philanthropy-data-commons-viewer ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
ffc8ec9
to
3c05c86
Compare
de5ca54
to
8b9c769
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat! Looks great.
I'm approving, but there are some small UX things that don't directly relate to lines of code and please take or leave:
- What would you think of removing closing punctuation from the link descriptions/ It seems a little odd but maybe that's just me
- It'd be cool to have a bolder visual hierarchy between the link titles and descritpions (making the descriptions slightly lighter, smaller, or making the titles slightly larger) right now it's hard for me to know the actual links at a glance / the descriptions make it harder to satisfice.
- When I have a drop down open would it be possible to allow me to swap to opening the other dropdown without having to click twice? e.g. right now:
(A) Click "About" (opens about
dropdown)
(B) Click "Developers" (closes about
dropdown)
(C) Click "Developers" again (opens developers
dropdown)
Would be nice to have:
(A) Click "About" (opens about
dropdown)
(B) Click "Developers" (closes about
dropdown and opens develoeprs
dropdown)
🎉
My rule about closing punctuation on repeated elements is to (1) be consistent across all items in the set and (2) omit them if possible, but (3) don't awkwardly contort language. I'd be happy to omit them from the descriptions, but some worked better with multiple sentences, so rule ① meant they all got them. I welcome edits to improve the language in a way that lets us omit them, but sounds like this is a light request anyway and not worthwhile!
Fair point! I intentionally mimicked the navbar link style for the titles, and I've lately been trying to minimize a visual hierarchy that ended up with inaccessible text sizes or contrasts. In this case my first pass actually did have the hierarchy you mentioned, but when I saw how relatively small the descriptions were, I reevaluated my biases to see if full-size/contrast text worked. I thought it actually ended up surprisingly fine… but I acknowledge that's visually subjective. Does the accessibility argument help?
I grok and agree with this desire. Initially I thought the implementation of this would be too difficult, but… I think I just realized how to do it fairly easily! Will implement… |
@slifty Actually… the item 3 is indeed gonna be more difficult than I'd hoped. I'm going to lean on your approval and just put this off for another day! Quick reason: our Dropdown component uses the neat trick of While there are all sorts of neat advantages to this implementation, one downside is you have to close the dropdown before using anything else. I was hoping to change the behavior specifically for navbar dropdowns, and nearly got there, but started to domino issues and hit my timebox. Thus, will need to save it for complaints. |
Massive rewrite of the `Dropdown` component to simplify its internals, improve its ergonomics, and allow its use in additional contexts such as the site navbar. - Add the explicit `DropdownTrigger` and `DropdownMenu` components, rather than implicity generating them from `Dropdown` props - Move left/right customization from `Dropdown` to `DropdownMenu` - Remove unused `className` hooks from all components; this is unused “flexibility” that is currently just unnecessary complexity - Make width configurable through CSS variables - Replace some unnecessarily BEM-style class names - Support both left- and right-aligned icons in `DropdownMenuLink`; while this is currently unused complexity, it also improves the ergonomics of the component and removes the forced horizontal flex layout from the internals The Storybook stories have also been updated accordingly. Issue #557 Modify Dropdown component to work with the site nav
In order for this not to cause collateral damage to base field short code formatting, this commit needed to also: - Include overrides for `.short-code` variants - Globalize the `.short-code` variant styling - Add that variant class to the proposal table context No direct issue, but this is leading up to the Developers menu, which will contain a pretty `<code>` element. Issue #559 Create Developer menu in navbar
Issue #559 Create Developer menu in navbar
Issue #558 Create About menu in navbar
8b9c769
to
4bfce8e
Compare
This PR reworks the
Dropdown
component to be useable with the site nav, and adds a Developers menu and an About menu:menus.mov
Note: The PR appears large because of the refactor of
Dropdown
, and because of some seemingly-unrelated (but actually prerequisite) changes from the changes to the<code>
element styling.Also: Please do suggest any edits to the wording of the menu items! The About menu items — which are just the links from the landing page, made available globally — largely use the already-approved wording from the landing page. But suggestions still welcome there, and the Developers menu is totally new.
Testing:
main
/production, note the lack of dropdown menus and the presence of the "Feedback" element in the navbar.REACT_APP_SHOW_STORYBOOK=true
, which is intended only to be the deploy preview environments.)Closes #557
Closes #558
Closes #559