-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat(plugins): Hides plugin tabs if no layers support them #2384
feat(plugins): Hides plugin tabs if no layers support them #2384
Conversation
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.
Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan, @DamonU2, and @kaminderpal)
packages/geoview-core/src/api/event-processors/event-processor-children/ui-event-processor.ts
line 50 at r1 (raw file):
static addHiddenTab(mapId: string, tab: string): void { if (!this.getUIState(mapId).hiddenTabs.includes(tab)) this.getUIState(mapId).setterActions.setHiddenTabs([...this.getUIState(mapId).hiddenTabs, tab]);
Maybe rename getUIState to getUIStateProtected like in other processor
5b26cb9
to
1afe129
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.
Reviewable status: 10 of 12 files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan, @jolevesq, and @kaminderpal)
packages/geoview-core/src/api/event-processors/event-processor-children/ui-event-processor.ts
line 50 at r1 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Maybe rename getUIState to getUIStateProtected like in other processor
Done.
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.
Reviewable status: 10 of 12 files reviewed, 3 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, and @jolevesq)
packages/geoview-core/src/ui/icon-button/icon-button.tsx
line 35 at r1 (raw file):
function getMaterialIconButton(): JSX.Element { return ( <span>
We dnt use span
/html element directly, we leverage Material UI Box
component.
Also please make sure, it doesn't break any functionality, i dnt remember fully, their was reason to remove span tag.
Code snippet:
<Box component="span">
</Box>
packages/geoview-core/src/ui/icon-button/icon-button.tsx
line 51 at r1 (raw file):
{children && children} </MaterialIconButton> </span>
Same span issue above.
1afe129
to
e312430
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.
Reviewed 10 of 12 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @DamonU2 and @jolevesq)
packages/geoview-core/src/ui/button/button.tsx
line 45 at r3 (raw file):
return ( <Tooltip title={t(tooltip || '')} placement={tooltipPlacement || 'bottom'} TransitionComponent={Fade}> <span>
Same comment from Kamy below for this line here
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.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @DamonU2 and @jolevesq)
packages/geoview-core/src/api/event-processors/event-processor-children/time-slider-event-processor.ts
line 91 at r3 (raw file):
// Make sure tab is visible UIEventProcessor.removeHiddenTab(mapId, 'time-slider');
Minor
I understand why it was coded the way it is and all. Makes sense.
That said, from an outsider perspective, reading 'remove hidden' is a bit weird as it's a bit like a double negation.
Suggestion to see if it couldn't be more like a tab visibility flag? visible/invisible.
showTab()
hideTab()
kind of functions?
e312430
to
09c3607
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.
Reviewable status: 7 of 15 files reviewed, 4 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, @jolevesq, and @kaminderpal)
packages/geoview-core/src/api/event-processors/event-processor-children/time-slider-event-processor.ts
line 91 at r3 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Minor
I understand why it was coded the way it is and all. Makes sense.
That said, from an outsider perspective, reading 'remove hidden' is a bit weird as it's a bit like a double negation.
Suggestion to see if it couldn't be more like a tab visibility flag? visible/invisible.
showTab()
hideTab()
kind of functions?
Done
packages/geoview-core/src/ui/button/button.tsx
line 45 at r3 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Same comment from Kamy below for this line here
Done.
packages/geoview-core/src/ui/icon-button/icon-button.tsx
line 35 at r1 (raw file):
Previously, kaminderpal (Kamy) wrote…
We dnt use
span
/html element directly, we leverage Material UIBox
component.Also please make sure, it doesn't break any functionality, i dnt remember fully, their was reason to remove span tag.
Done.
packages/geoview-core/src/ui/icon-button/icon-button.tsx
line 51 at r1 (raw file):
Previously, kaminderpal (Kamy) wrote…
Same span issue above.
Done.
09c3607
to
2f156ca
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.
Reviewed 1 of 2 files at r2, 1 of 1 files at r3, 8 of 8 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan)
e4a8e73
into
Canadian-Geospatial-Platform:develop
Closes #2369
Description
Removes (hides) tabs for time slider and geochart from footer-bar if there are no layers that support them. Also adds name and email to type action to stabilize it, and span tags to buttons to avoid errors when they are disabled.
Type of change
How Has This Been Tested?
https://damonu2.github.io/geoview/demos-navigator.html?config=./configs/navigator/10-package-time-slider.json
https://damonu2.github.io/geoview/demos-navigator.html?config=./configs/navigator/12-package-geochart.json
https://damonu2.github.io/geoview/add-layers.html
Checklist:
I have made corresponding changes to the documentationI have added tests that prove my fix is effective or that my feature worksNew and existing unit tests pass locally with my changesThis change is