-
Notifications
You must be signed in to change notification settings - Fork 38
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
fix(ui): reorganize ui component to follow best pratices #2754
fix(ui): reorganize ui component to follow best pratices #2754
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 79 of 79 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Alex-NRCan and @DamonU2)
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 79 files at r1, 5 of 10 files at r2, all commit messages.
Reviewable status: 74 of 79 files reviewed, 3 unresolved discussions (waiting on @DamonU2 and @jolevesq)
packages/geoview-core/src/ui/accordion/accordion.tsx
line 51 at r2 (raw file):
// Define AccordionExpandIcon outside of the main component const AccordionExpandIcon = memo(function AccordionExpandIcon({
I'm not sure how I feel about memoizing UI components meant to be reusable in the framework? In my mind, memo usage is more 'applicative' than 'framework' related.
packages/geoview-core/src/ui/accordion/accordion.tsx
line 70 at r2 (raw file):
* @returns {JSX.Element} the created Fade element */ export const Accordion = memo(function Accordion(props: AccordionProps): ReactNode {
Same comment
packages/geoview-core/src/ui/button/button.tsx
line 98 at r2 (raw file):
// Export the Button using forwardRef so that passing ref is permitted and functional in the react standards export const Button = memo(forwardRef<HTMLButtonElement, ButtonProps>(MaterialBtn));
Same comment
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 29 of 79 files at r1, 4 of 10 files at r2.
Reviewable status: 78 of 79 files reviewed, 5 unresolved discussions (waiting on @DamonU2 and @jolevesq)
packages/geoview-core/src/ui/paper/paper.tsx
line 20 at r2 (raw file):
// Export the Paper using forwardRef so that passing ref is permitted and functional in the react standards export const Paper = memo(forwardRef<HTMLDivElement, PaperProps>(MUIPaper));
Same comment, I'd remove the memo usage here and maybe revert the whole file.
packages/geoview-core/src/ui/panel/panel.tsx
line 29 at r2 (raw file):
// Callback when the user clicked the general close button handleGeneralClose?: () => void; // Callback when the panel has completed opened (and transitioned in)
The best practice is to use the 'on' prefix rather than 'handle' for callbacks sent as props in a component.
The handle prefix is to be used when defining a function inside a component. I'd leave them as 'on' and change 'handleKeyDown' to 'onKeyDown'.
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 69 of 79 files at r1, 10 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @jolevesq)
packages/geoview-core/src/ui/slider/slider.tsx
line 241 at r2 (raw file):
useEffect(() => { logger.logTraceUseEffect('UI.SLIDER - foscus when mount');
foscus > focus
packages/geoview-core/src/ui/modal/modal-api.ts
line 49 at r2 (raw file):
* Function that deletes the modal by the id specified * * @param { string } modalId of the modal that is to be deleted
modalId - ID of the...
packages/geoview-core/src/ui/modal/modal-api.ts
line 58 at r2 (raw file):
* Function that open the modal by the id specified * * @param { string } modalId of the modal that is to be deleted
modalId - ID of the...
packages/geoview-core/src/ui/modal/modal-api.ts
line 69 at r2 (raw file):
* Function that close the modal by the id specified * * @param { string } modalId of the modal that is to be deleted
modalId - ID of the...
2db3b31
into
Canadian-Geospatial-Platform:develop
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #2743
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Hosted Feb 17, 16h30
https://jolevesq.github.io/geoview/demos-navigator.html?config=./configs/navigator/19-geojson.json
Need to play with many demo as ui components are used in many places
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