[UX] Consolidate menu bars #1586
Merged
Conversation
This file contains 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
joshuarrrr
force-pushed
the
ux/header-simplify
branch
4 times, most recently
from
bb2b765 to
d2a2e09
Compare
|
One issue discovered is that the reporting plugin has hard-coded the popover positioning, so I may need a follow-up PR there to fix that in their repo, and is something to call out for plugin authors in general: @kgcreative also pointed out that there's some inconsistency with popover positioning - we may need to revisit the default positioning to ensure consistency (the preference is for the underneath popover): Also, the reporting menu action item doesn't appear in the mobile menu: |
|
Functional test failures currently under investigation: |
joshuarrrr
force-pushed
the
ux/header-simplify
branch
from
cb03814 to
3e6c7d5
Compare
Codecov Report
@@ Coverage Diff @@
## main #1586 +/- ##
==========================================
- Coverage 67.51% 67.50% -0.02%
==========================================
Files 3073 3074 +1
Lines 59069 59127 +58
Branches 8963 8988 +25
==========================================
+ Hits 39881 39911 +30
- Misses 17005 17031 +26
- Partials 2183 2185 +2
Continue to review full report at Codecov.
|
tmarkley
reviewed
May 19, 2022
There was a problem hiding this comment.
Looks pretty good! Can we delete this file as well now? https://github.com/tmarkley/OpenSearch-Dashboards/blob/c74c1f57d119f63462fdbd3fde5986957bc8a79a/src/core/server/core_app/assets/default_branding/opensearch_logo.svg
| @@ -221,15 +221,37 @@ export default function ({ getService, getPageObjects }) { | |||
| await PageObjects.visEditor.changeYAxisFilterLabelsCheckbox(axisId, false); | |||
| await PageObjects.visEditor.clickGo(); | |||
| const labels = await PageObjects.visChart.getYAxisLabels(); | |||
| const expectedLabels = ['0', '2,000', '4,000', '6,000', '8,000', '10,000']; | |||
| const expectedLabels = [ | |||
There was a problem hiding this comment.
Did these change because of the additional space?
There was a problem hiding this comment.
Yeah, just enough vertical room at whatever display height the functional tests run at to allow for the extra ticks.
joshuarrrr
force-pushed
the
ux/header-simplify
branch
from
c100d33 to
20132ce
Compare
|
Did we want to convert this to draft since it's not targeting 2.0? Or close it since it's being reworked? |
joshuarrrr
force-pushed
the
ux/header-simplify
branch
from
20132ce to
2a65dfb
Compare
Although we're not currently using it anywhere, I don't think there's a need to remove it from the repo altogether; my understanding is that @kgcreative may decide to use it elsewhere (possibly on the loading or welcome screens). |
joshuarrrr
force-pushed
the
ux/header-simplify
branch
2 times, most recently
from
ea2e3e6 to
22bbaf9
Compare
joshuarrrr
commented
Jun 15, 2022
Comment on lines
-220
to
-253
| {/* Pinned items */} | ||
| <EuiFlexItem grow={false} style={{ flexShrink: 0 }}> | ||
| <EuiCollapsibleNavGroup | ||
| background="light" | ||
| className="eui-yScroll" | ||
| style={{ maxHeight: '40vh' }} | ||
| > | ||
| <EuiListGroup | ||
| aria-label={i18n.translate('core.ui.primaryNav.pinnedLinksAriaLabel', { | ||
| defaultMessage: 'Pinned links', | ||
| })} | ||
| listItems={[ | ||
| { | ||
| label: 'Home', | ||
| iconType: 'home', | ||
| href: homeHref, | ||
| onClick: (event) => { | ||
| if (isModifiedOrPrevented(event)) { | ||
| return; | ||
| } | ||
|
|
||
| event.preventDefault(); | ||
| closeNav(); | ||
| navigateToApp('home'); | ||
| }, | ||
| }, | ||
| ]} | ||
| maxWidth="none" | ||
| color="text" | ||
| gutterSize="none" | ||
| size="s" | ||
| /> | ||
| </EuiCollapsibleNavGroup> | ||
| </EuiFlexItem> |
There was a problem hiding this comment.
Removing home link from the collapsible menu.
| /> | ||
| {useExpandedMenu && ( | ||
| <EuiHeader | ||
| theme="dark" |
There was a problem hiding this comment.
It's a bit weird that we kind of misuse the theme to hard-code this expanded menu as dark, but there's no good reason to change, especially now that it won't be enabled by default.
Comment on lines
132
to
155
| ...(observables.navControlsExpandedCenter$ && { | ||
| items: [ | ||
| <EuiShowFor sizes={['m', 'l', 'xl']}> | ||
| <HeaderNavControls navControls$={observables.navControlsExpandedCenter$} /> | ||
| </EuiShowFor>, | ||
| ], | ||
| }), | ||
| borders: 'none', | ||
| }, | ||
| { | ||
| ...((observables.navControlsExpandedCenter$ || | ||
| observables.navControlsExpandedRight$) && { | ||
| items: [ | ||
| <EuiHideFor sizes={['m', 'l', 'xl']}> | ||
| <HeaderNavControls navControls$={observables.navControlsExpandedCenter$} /> | ||
| </EuiHideFor>, | ||
| <HeaderNavControls navControls$={observables.navControlsExpandedRight$} />, | ||
| ], | ||
| }), | ||
| borders: 'none', | ||
| }, |
There was a problem hiding this comment.
This will be a breaking change for Bitergia - they'll need to update their menu plugin to uses these new navControls locations. But because we're going to the simplified menu by default, I think that's the best migration path (and better than trying to find some conditional solution) - any other plugins using the existing navControls locations won't need to do anything.
Comment on lines
30
to
36
| const getIconProps = () => { | ||
| const iconType = customMark | ||
| ? customMark | ||
| : useExpandedMenu | ||
| ? 'home' | ||
| : `${assetFolderUrl}/${defaultMark}`; | ||
| const testSubj = customMark ? 'customLogo' : useExpandedMenu ? 'homeLogo' : 'defaultLogo'; |
There was a problem hiding this comment.
I generally don't like nested ternaries, but I didn't particularly like the other options here either. Essentially, we'll always use a custom mark if provided; otherwise we'll use the OSD logo in the default mode or a home icon in the expanded mode.
Open to suggestions!
Comment on lines
+37
to
+39
| .headerIsExpanded & { | ||
| min-height: calc(100vh - #{$euiHeaderHeightCompensation * 2}); | ||
| } |
There was a problem hiding this comment.
It seems wrong to need to have this calculation done imperatively at the plugin level. But we don't have access to variables defined in the core, such as $osdHeaderOffset. I explored the idea of using root CSS variables that are reset, instead of toggling the body class, but this seemed like the safest and least invasive approach. 😧
kavilla
reviewed
Jun 16, 2022
|
Looking cool! However, even though I believe this is by design, the top logo not being clickable in the expanded view feels a little weird especially if existing UX would do that. |
kavilla
reviewed
Jun 16, 2022
We should make the top logo clickable, even if we do have the bottom bar also go "home". |
joshuarrrr
force-pushed
the
ux/header-simplify
branch
from
ef78e8f to
3051ba6
Compare
Done. |
kavilla
previously approved these changes
Jun 23, 2022
|
LGTM! In the doc issue we can add more details about the plan switch over? Like we plan to consolidate by default for |
ashwin-pc
previously approved these changes
Jun 23, 2022
- update branding mark unit tests - fix header spacing at smallest breakpoint - update custom branding functional test - update line chart functional test ticks to account for more ticks visisble with larger height of viz area - update release notes Signed-off-by: Josh Romero <[email protected]>
- add branding config for expanded menu Signed-off-by: Josh Romero <[email protected]>
- revert release note addition Signed-off-by: Josh Romero <[email protected]>
- remove home link from collapsible nav Signed-off-by: Josh Romero <[email protected]>
- Add expanded header option - Move navControlsRight to left of help - Rename HeaderLogo to HomeLoader - Serves as unified home button and global loading indicator - Add title for tooltip, because purpose may not be obvious - Rename Mark to HomeIcon - Uses home icon when header expanded by default - Restore HeaderLogo for expanded header - Duplicate default logo, rename to match mark naming (default, dark) - Add new nav control areas for expanded menu only - navControlsExpandedRight - navControlsExpandedCenter - Add new body class for expanded header - used by styles to correctly set app height - remove unecessary duplicate header mixin inclusion Signed-off-by: Josh Romero <[email protected]>
- fix functional tests - update test subjects - convert home tests to ts - update names and methods to be more clear Signed-off-by: Josh Romero <[email protected]>
- update snapshots Signed-off-by: Josh Romero <[email protected]>
- add, update, improve unit tests Signed-off-by: Josh Romero <[email protected]>
- keep expanded as default config Signed-off-by: Josh Romero <[email protected]>
- restore logo link - update functional tests Signed-off-by: Josh Romero <[email protected]>
- group dark mode branding tests Signed-off-by: Josh Romero <[email protected]>
- final review updates Signed-off-by: Josh Romero <[email protected]>
joshuarrrr
force-pushed
the
ux/header-simplify
branch
from
0be36bc to
a8d9e71
Compare
|
I'll squash the commit message when merging: |
ashwin-pc
approved these changes
Jun 24, 2022
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Jun 24, 2022
Add a config option to remove the upper global header and incorporate all items into a single header. While the previous expanded dual header is still the default, we've added a new home button which also serves as the location of the global loading indicator. Additional changes: * update branding unit tests * fix header spacing at smallest breakpoint * update branding functional tests Issue resolved: #1583 Squashed commits: - update branding mark unit tests - fix header spacing at smallest breakpoint - update custom branding functional test - update line chart functional test ticks to account for more ticks visisble with larger height of viz area - update release notes - add branding config for expanded menu - revert release note addition - remove home link from collapsible nav - Add expanded header option - Move navControlsRight to left of help - Rename HeaderLogo to HomeLoader - Serves as unified home button and global loading indicator - Add title for tooltip, because purpose may not be obvious - Rename Mark to HomeIcon - Uses home icon when header expanded by default - Restore HeaderLogo for expanded header - Duplicate default logo, rename to match mark naming (default, dark) - Add new nav control areas for expanded menu only - navControlsExpandedRight - navControlsExpandedCenter - Add new body class for expanded header - used by styles to correctly set app height - remove unecessary duplicate header mixin inclusion - fix functional tests - update test subjects - convert home tests to ts - update names and methods to be more clear - update snapshots - add, update, improve unit tests - keep expanded as default config - restore logo link - update functional tests - group dark mode branding tests - final review updates Signed-off-by: Josh Romero <[email protected]> (cherry picked from commit ae6cb80)
kavilla
pushed a commit
that referenced
this pull request
Jun 24, 2022
Add a config option to remove the upper global header and incorporate all items into a single header. While the previous expanded dual header is still the default, we've added a new home button which also serves as the location of the global loading indicator. Additional changes: * update branding unit tests * fix header spacing at smallest breakpoint * update branding functional tests Issue resolved: #1583 Squashed commits: - update branding mark unit tests - fix header spacing at smallest breakpoint - update custom branding functional test - update line chart functional test ticks to account for more ticks visisble with larger height of viz area - update release notes - add branding config for expanded menu - revert release note addition - remove home link from collapsible nav - Add expanded header option - Move navControlsRight to left of help - Rename HeaderLogo to HomeLoader - Serves as unified home button and global loading indicator - Add title for tooltip, because purpose may not be obvious - Rename Mark to HomeIcon - Uses home icon when header expanded by default - Restore HeaderLogo for expanded header - Duplicate default logo, rename to match mark naming (default, dark) - Add new nav control areas for expanded menu only - navControlsExpandedRight - navControlsExpandedCenter - Add new body class for expanded header - used by styles to correctly set app height - remove unecessary duplicate header mixin inclusion - fix functional tests - update test subjects - convert home tests to ts - update names and methods to be more clear - update snapshots - add, update, improve unit tests - keep expanded as default config - restore logo link - update functional tests - group dark mode branding tests - final review updates Signed-off-by: Josh Romero <[email protected]> (cherry picked from commit ae6cb80) Co-authored-by: Josh Romero <[email protected]>
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.
Description
Add a config option to remove the upper global header and incorporate all items into a single header. While the previous expanded dual header is still the default, we've added a new home button which also serves as the location of the global loading indicator.
Additional changes:
Issues Resolved
Fixes #1583
Check List
yarn test:jestyarn test:jest_integrationyarn test:ftr