🪟🎨Main menu hover state #13762
Merged
🪟🎨Main menu hover state #13762
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
area/platform
timroes
reviewed
Jun 15, 2022
timroes
reviewed
Jun 15, 2022
| const { location } = useRouter(); | ||
|
|
||
| const menuItemStyle = (isActive: boolean) => { | ||
| const isChild = location.pathname.split("/").length > 4 && !location.pathname.includes("settings"); |
There was a problem hiding this comment.
This feels like a very specific hack for settings. Is there any reason why we would want to exclude this behavior for settings specifically? I would prefer if we don't need to add page specific hacks here, and could instead stick with a more generic implementation here.
There was a problem hiding this comment.
Settings doesn't have a "landing page" (/settings). I've asked Nico for some feedback there (if we'd like it to act like selected or parentSelected).
There was a problem hiding this comment.
We could check if the menuItem's formatted message lowercased equals the last item in the pathname? I don't think that works well with i18n though. Any page with no "landing" page would never have isChild be true... Settings is just the only one we have right now.
teallarson
force-pushed
the
teal/main-menu-hover-state
branch
from
5b2046f to
f3d8ace
Compare
edmundito
reviewed
Jun 28, 2022
airbyte-webapp/src/packages/cloud/views/layout/SideBar/SideBar.tsx
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/packages/cloud/views/layout/SideBar/SideBar.tsx
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/packages/cloud/views/layout/SideBar/SideBar.tsx
Outdated
Show resolved
Hide resolved
github-actions
bot
added
area/connectors
octavia-squidington-iii
added
connectors/destination/s3
connectors/destination/databricks
connectors/destination/gcs
connectors/source/clickhouse
connectors/source/github
connectors/source/file
connectors/source/mssql
connectors/source/postgres
connectors/source/zendesk-chat
connectors/source/jdbc
connectors/source/relational-db
connectors/source/clickhouse-strict-encrypt
labels
teallarson
force-pushed
the
teal/main-menu-hover-state
branch
from
0dc0561 to
ce03d04
Compare
github-actions
bot
removed
area/documentation
Co-authored-by: Edmundo Ruiz Ghanem <[email protected]>
…om/airbytehq/airbyte into teal/main-menu-hover-state" This reverts commit 423f671, reversing changes made to 38a39a3.
teallarson
force-pushed
the
teal/main-menu-hover-state
branch
from
7742d58 to
2ce4e45
Compare
edmundito
approved these changes
Jul 14, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
closes #12725
Adds a hover state and adds additional functionality to the active state in the SideBar menu.
Note: for this PR, we will not be implementing the "selected" state on the flyout menus. The flyout menus (a) currently do not expose if they are "active" and (b) are due for a revisit as a whole (accessibility and design issues). When we recompose them, we can expose whether the flyout is open or not, making the active state simpler to implement.
How
MenuItemto an scss classRecommended reading order
/project/cloud)Testing
If you are testing this locally, it's worth noting that Cloud and OSS currently each have their own SideBar. For this PR, you will want to test each environment separately. (there is an issue to create a more centralized SideBar component but was out of scope here)