-
Notifications
You must be signed in to change notification settings - Fork 36
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
Refactor/render top toolbar #2694
base: develop
Are you sure you want to change the base?
Refactor/render top toolbar #2694
Conversation
48aa1fa
to
b4847fc
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 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @DamonU2 and @PVautour)
packages/geoview-aoi-panel/src/aoi-panel.tsx
line 1 at r1 (raw file):
/* eslint react/prop-types: 0 */
I am a bit scare to enter a modification for a build that is working for everybody else....
packages/.eslintrc
line 35 at r1 (raw file):
}, "rules": { "react/prop-types": "off",
Why have you added this?
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.
Click on hide column turn off visibility but toggle tays on
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @DamonU2 and @PVautour)
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 4 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @PVautour)
packages/geoview-aoi-panel/src/aoi-panel.tsx
line 1 at r1 (raw file):
/* eslint react/prop-types: 0 */
Is this necessary if we're turning it off in the config file? Also, should we be turning it off in the config file?
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, 3 unresolved discussions (waiting on @DamonU2 and @PVautour)
packages/geoview-aoi-panel/src/aoi-panel.tsx
line 1 at r1 (raw file):
Previously, DamonU2 (Damon Ulmi) wrote…
Is this necessary if we're turning it off in the config file? Also, should we be turning it off in the config file?
What do you mean @DamonU2 by turning it off from config?
Previously, jolevesq (Johann Levesque) wrote…
Damon you are correct this does not need to be here. It is redundant. |
Previously, jolevesq (Johann Levesque) wrote…
Here is a snippet from the official react upgrade guide:
Source: https://react.dev/blog/2024/04/25/react-19-upgrade-guide#removed-proptypes-and-defaultprops Is it ok if I disable it even if we are on 18? It's deprecated anyway. |
…or performance gains.
b4847fc
to
64ff052
Compare
Description
Move top toolbar rendering into a separate file for improved readability and performance gains in dev.
Fixes # (issue)
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.
https://pvautour.github.io/geoview/outlier-ESRI-maxRecordCount.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