-
Notifications
You must be signed in to change notification settings - Fork 37
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(legend) Ordering issues with store #2726
base: develop
Are you sure you want to change the base?
fix(legend) Ordering issues with store #2726
Conversation
f59c593
to
26bb866
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 9 of 9 files at r1, 22 of 22 files at r2, 19 of 19 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Alex-NRCan)
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 r4, 34 of 34 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @jolevesq)
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 9 files at r1, 8 of 22 files at r2, 13 of 19 files at r3, 11 of 12 files at r4, 34 of 34 files at r5, all commit messages.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @DamonU2)
packages/geoview-core/src/core/app-start.tsx
line 56 at r5 (raw file):
}, [mapId]); // GV get store values by id because context is not set.... it is the only 2 atomic selector by id
should say 1 atomic selector
packages/geoview-core/src/core/app-start.tsx
line 58 at r5 (raw file):
// GV get store values by id because context is not set.... it is the only 2 atomic selector by id // once context is define, map id is available const theme = useAppDisplayThemeById(mapId);
I think this store hook was only used here, have you deleted it in the store?
packages/geoview-core/src/core/components/layers/left-panel/single-layer.tsx
line 83 at r5 (raw file):
const legendExpanded = !useSelectorLayerLegendCollapsed(mapId, layer.layerPath); const listItemSpring = useSpring({
Can we remove the animation?
packages/geoview-core/src/core/components/layers/left-panel/single-layer.tsx
line 92 at r5 (raw file):
const isLayerChildSelected = useCallback( (startingLayer: TypeLegendLayer): boolean => { // Log
Remove // Log ?
packages/geoview-core/src/core/components/layers/left-panel/single-layer.tsx
line 115 at r5 (raw file):
// returns true if any of the layer children has visibility of false const layerHasDisabledVisibility = useCallback((startingLayer: TypeLegendLayer): boolean => { // Log
Remove // Log ?
packages/geoview-core/src/core/components/common/layer-list.tsx
line 87 at r5 (raw file):
const handleLayerKeyDown = useCallback( (event: React.KeyboardEvent, selectedLayer: LayerListEntry): void => { // Log
I mostly removed the // Log comment everywhere. I find it easier to read and logger is mostly always the first line
packages/geoview-core/src/core/components/layers/left-panel/layers-list.tsx
line 8 at r5 (raw file):
import { TypeLegendLayer } from '@/core/components/layers/types'; import { TABS } from '@/core/utils/constant'; import { SingleLayer } from './single-layer';
Do you want to use relative path like elsewhere?
packages/geoview-core/src/core/components/layers/left-panel/layers-list.tsx
line 33 at r5 (raw file):
}); // TODO: Check - This sort should likely happen elsewhere than in a rendering component
Agree, should happen when layer are reordered and (delete/added). If this sortedLayers is use as dependencies it forces the re rendering
packages/geoview-core/src/core/components/legend/legend-layer-ctrl.tsx
line 93 at r5 (raw file):
// Log logger.logTraceRender('components/legend/legend-layer-ctrl', layerPath, isVisible);
There is double renderer log, is it indented? If so we should add a message so it is not deleted later
packages/geoview-core/src/core/components/legend/legend.tsx
line 18 at r5 (raw file):
import { TypeLegendLayer } from '@/core/components/layers/types'; import { CONTAINER_TYPE } from '@/core/utils/constant'; import { useDebounceLayerLegendLayers } from './hooks/use-legend-debounce';
We put relative? I saw you did for some... We said we accept ./ but it may be better to standardize and always use @
packages/geoview-core/src/geo/layer/gv-layers/raster/gv-esri-dynamic.ts
line 26 at r5 (raw file):
} from '@/geo/map/map-schema-types'; import { esriGetFieldType, esriGetFieldDomain } from '@/geo/layer/gv-layers/utils'; import { AbstractGVRaster } from './abstract-gv-raster';
Change ./ ?
packages/geoview-core/src/core/components/layers/layers-panel.tsx
line 33 at r5 (raw file):
const showLayerDetailsPanel = useCallback( (layerId: string): void => {
Do we need a logger for use callback?
packages/geoview-core/src/geo/layer/layer-sets/legends-layer-set.ts
line 5 at r5 (raw file):
import { logger } from '@/core/utils/logger'; import { TypeLayerStatus } from '@/geo/map/map-schema-types'; import { AbstractLayerSet, PropagationType } from './abstract-layer-set';
change ,/ ?
packages/geoview-core/src/core/components/layers/left-panel/delete-undo-button.tsx
line 63 at r5 (raw file):
const { setOrToggleLayerVisibility } = useMapStoreActions(); const { setSelectedFooterLayerListItemId } = useUIStoreActions(); const isVisible = useSelectorLayerVisibility(mapId, layerPath);
mapId is not already available within the store state?
packages/geoview-core/src/core/components/layers/left-panel/delete-undo-button.tsx
line 77 at r5 (raw file):
}; const handleDeleteKeyDown = (e: KeyboardEvent): void => {
Should we rename e to event?
packages/geoview-core/src/geo/layer/gv-layers/tile/gv-xyz-tiles.ts
line 7 at r5 (raw file):
import { XYZTilesLayerEntryConfig } from '@/core/utils/config/validation-classes/raster-validation-classes/xyz-layer-entry-config'; import { AbstractGVTile } from './abstract-gv-tile';
Change ./ ?
packages/geoview-core/src/ui/list/checkbox-list/checkbox-list.tsx
line 73 at r5 (raw file):
* @param e React.MouseEvent<HTMLElement> The mouse click event */ const handleClickContent = useCallback((event: React.MouseEvent<HTMLElement>): void => {
We need a logger?
packages/geoview-core/src/core/containers/focus-trap.tsx
line 10 at r5 (raw file):
import { Modal, Button } from '@/ui'; import { UseHtmlToReact } from '@/core/components/common/hooks/use-html-to-react'; import { getFocusTrapSxClasses } from './containers-style';
Do we keep ./
packages/geoview-core/src/ui/drawer/drawer.tsx
line 9 at r5 (raw file):
import { IconButton, ChevronLeftIcon, ChevronRightIcon } from '@/ui'; import { logger } from '@/core/utils/logger'; import { getSxClasses } from './drawer-style';
Change ./ ?
packages/geoview-core/src/geo/layer/gv-layers/raster/gv-esri-image.ts
line 20 at r5 (raw file):
import { validateExtent } from '@/geo/utils/utilities'; import { getLegendStyles } from '@/geo/utils/renderer/geoview-renderer'; import { AbstractGVRaster } from './abstract-gv-raster';
Change ./ ?
packages/geoview-core/src/core/workers/abstract-worker.ts
line 3 at r5 (raw file):
import { wrap, Remote } from 'comlink'; import { logger } from '@/core/utils/logger'; import { WorkerLogLevel } from './helper/logger-worker';
Change ./ ?
packages/geoview-core/src/geo/layer/layer-sets/all-feature-info-layer-set.ts
line 6 at r5 (raw file):
import { AbstractBaseLayer } from '@/geo/layer/gv-layers/abstract-base-layer'; import { GVWMS } from '@/geo/layer/gv-layers/raster/gv-wms'; import { AbstractLayerSet, PropagationType } from './abstract-layer-set';
Change ./ ?
packages/geoview-core/src/core/stores/state-api.ts
line 6 at r5 (raw file):
import { SwiperEventProcessor } from '@/api/event-processors/event-processor-children/swiper-event-processor'; import { TimeSliderEventProcessor } from '@/api/event-processors/event-processor-children/time-slider-event-processor'; import { GeoChartStoreByLayerPath, TypeGeochartResultSetEntry } from './store-interface-and-intial-values/geochart-state';
Do we change all the ./ in this file?
packages/geoview-core/src/ui/modal/modal.tsx
line 17 at r5 (raw file):
import { CloseIcon, IconButton } from '@/ui'; import { getSxClasses } from './modal-style';
Change ./ ?
packages/geoview-core/src/core/components/legend/legend-layer.tsx
line 75 at r5 (raw file):
const { setLegendCollapsed } = useMapStoreActions(); const { getLayerStatus } = useLayerStoreActions(); const isVisible = useSelectorLayerVisibility(mapId, layer.layerPath);
Same as before, mapId should be accessible from within the state itself?
Description
Quick PR to help fixing the ordering issues:
Type of change
How Has This Been Tested?
Hosted: Feb. 14th @ 15h30 : https://alex-nrcan.github.io/geoview/
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![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)