-
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(datatable): common enlarge and close btn #1494 #1500
feat(datatable): common enlarge and close btn #1494 #1500
Conversation
…form/geoview into 1494-enlarge-close-list
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 15 files at r1, 13 of 13 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @kaminderpal)
packages/geoview-core/src/core/components/common/close-button.tsx
line 19 at r2 (raw file):
*/ export function CloseButton({ isLayersPanelVisible, setIsLayersPanelVisible }: CloseButtonProps) { const { t } = useTranslation();
Can you regroup like the best practice doc https://github.com/Canadian-Geospatial-Platform/geoview/blob/develop/docs/programming/best-practices.md
const { t } = useTranslation<string>();
const theme = useTheme();
const sxClasses = getSxClasses(theme);
packages/geoview-core/src/core/components/common/enlarge-button.tsx
line 19 at r2 (raw file):
*/ export function EnlargeButton({ isEnlargeDataTable, setIsEnlargeDataTable }: EnlargeButtonProps) { const { t } = useTranslation();
Can you regroup like the best practice doc https://github.com/Canadian-Geospatial-Platform/geoview/blob/develop/docs/programming/best-practices.md
const { t } = useTranslation<string>();
const theme = useTheme();
const sxClasses = getSxClasses(theme);
packages/geoview-core/src/core/components/common/layer-list.tsx
line 34 at r2 (raw file):
} export function LayerList({
Can you add JSDOC comment for this function
packages/geoview-core/src/core/components/common/layer-list.tsx
line 42 at r2 (raw file):
mapFiltered = {}, }: LayerListProps) { const { t } = useTranslation();
Can you regroup like the best practice doc https://github.com/Canadian-Geospatial-Platform/geoview/blob/develop/docs/programming/best-practices.md
const { t } = useTranslation<string>();
const theme = useTheme();
const sxClasses = getSxClasses(theme);
packages/geoview-core/src/core/components/common/layer-list.tsx
line 62 at r2 (raw file):
const getFeaturesOfLayer = (layerPath: string, index: number): string => { return rowsFiltered && rowsFiltered[layerPath] ? `${rowsFiltered[layerPath]} ${t('dataTable.featureFiltered')}`
This is specific to datatable.... should it be set by the component itself (details, table, slider, ...) Maybe each component can set his own info in the store to be reuse by this component?
packages/geoview-core/src/core/components/common/layer-list.tsx
line 104 at r2 (raw file):
</Typography> {isMapFilteredSelectedForLayer(layer.layerPath) && <FilterAltIcon sx={{ color: theme.palette.grey['500'] }} />}
Is this specific to data table? Can we have generic placeholder here and in table set the icon and stuff. This way this component stay very generic
packages/geoview-core/src/core/components/common/responsive-grid.tsx
line 2 at r2 (raw file):
import { ReactNode } from 'react'; import { GridProps, SxProps } from '@mui/material';
Should these 2 import came from the Grid element in UI. For the moment I think Grid is imported directly from Material. should we create the component and inject these import?
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: 11 of 19 files reviewed, 7 unresolved discussions (waiting on @jolevesq)
packages/geoview-core/src/core/components/common/close-button.tsx
line 19 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Can you regroup like the best practice doc https://github.com/Canadian-Geospatial-Platform/geoview/blob/develop/docs/programming/best-practices.md
const { t } = useTranslation<string>(); const theme = useTheme(); const sxClasses = getSxClasses(theme);
Done.
packages/geoview-core/src/core/components/common/enlarge-button.tsx
line 19 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Can you regroup like the best practice doc https://github.com/Canadian-Geospatial-Platform/geoview/blob/develop/docs/programming/best-practices.md
const { t } = useTranslation<string>(); const theme = useTheme(); const sxClasses = getSxClasses(theme);
Done.
packages/geoview-core/src/core/components/common/layer-list.tsx
line 34 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Can you add JSDOC comment for this function
Done.
packages/geoview-core/src/core/components/common/layer-list.tsx
line 42 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Can you regroup like the best practice doc https://github.com/Canadian-Geospatial-Platform/geoview/blob/develop/docs/programming/best-practices.md
const { t } = useTranslation<string>(); const theme = useTheme(); const sxClasses = getSxClasses(theme);
Done.
packages/geoview-core/src/core/components/common/layer-list.tsx
line 62 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
This is specific to datatable.... should it be set by the component itself (details, table, slider, ...) Maybe each component can set his own info in the store to be reuse by this component?
done
packages/geoview-core/src/core/components/common/layer-list.tsx
line 104 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this specific to data table? Can we have generic placeholder here and in table set the icon and stuff. This way this component stay very generic
Done.
packages/geoview-core/src/core/components/common/responsive-grid.tsx
line 2 at r2 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should these 2 import came from the Grid element in UI. For the moment I think Grid is imported directly from Material. should we create the component and inject these import?
Done.
…form/geoview into 1494-enlarge-close-list
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 13 files at r2, 9 of 9 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @kaminderpal)
packages/geoview-core/src/core/components/data-table/data-panel.tsx
line 155 at r3 (raw file):
<ResponsiveGrid.Left isLayersPanelVisible={isLayersPanelVisible} xs={isLayersPanelVisible ? 12 : 0}
The break points are the smae in all component, dhould it be default in ResponsiveGrid.Right and optional to override if needed?
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 7 of 7 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @kaminderpal)
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 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @kaminderpal)
c5eeacf
into
Canadian-Geospatial-Platform:develop
Description
Create reusable enlarge, close and layer list component.
Fixes # (1494)
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.
Add the URL for your deploy!
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