Conversation
1042239 to
45ce728
Compare
There was a problem hiding this comment.
Copilot reviewed 73 out of 73 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (4)
packages/utah-design-system/src/components/Avatar.tsx:22
- Switching from the wrapper Boolean to the primitive boolean improves type safety. Please confirm that this change is aligned with the expected type definitions across the component.
anonymous: boolean;
packages/utah-design-system/src/components/AlertDialog.tsx:54
- The removal of autoFocus from the Button component may impact focus management and accessibility when the dialog renders. Please ensure that focus is being handled appropriately elsewhere.
autoFocus
.storybook/preview.js:5
- The change of the firebaseConfig import path from the production folder to a tests folder suggests a switch to a test configuration. Please verify that this change is intentional and will not lead to unexpected behavior in Storybook.
import { firebaseConfig } from '../packages/utah-design-system/tests/firebase';
packages/layer-selector/src/Discover.js:8
- [nitpick] The magic number '20' used in the slice remains opaque. Consider replacing it with a well-named constant to improve clarity and maintainability.
const fiveToNineteen = lods.slice(0, 20);
d540703 to
546730a
Compare
There was a problem hiding this comment.
Copilot reviewed 75 out of 75 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
packages/utah-design-system/src/components/AlertDialog.tsx:54
- Removing the autoFocus prop from the Button may impact focus management and accessibility for the AlertDialog. Please verify that this change is intentional and that the dialog still meets accessibility requirements.
autoFocus
|
Visit the preview URL for this PR (updated for commit d03947c): |
7f92444 to
8305a5f
Compare
|
@steveoh This is ready for a closer look. I've tested this in spills and atlas and it seems to work well. There is a bit of an upgrade path but the types make it pretty easy. I wondered about jamming everything into a single file since the new version is far fewer lines. There are also some features that are not supported. You cannot mark a basemap as selected by default. It's always the first one. The code to make the random basemap work in atlas would look something like this: const selectorOptions: LayerSelectorProps = {
options: {
view: mapView.current,
quadWord: import.meta.env.VITE_DISCOVER,
baseLayers: [
'Hybrid',
{
label: 'Lite',
function: () =>
new VectorTileLayer({
url: urls.liteVector,
opacity: 1,
}),
},
'Terrain',
'Topo',
'Color IR',
],
referenceLayers: ['Address Points', 'Land Ownership'],
},
};
const { index: randomBaseMapIndex } = randomize(selectorOptions.options.baseLayers);
const removed = selectorOptions.options.baseLayers.splice(randomBaseMapIndex, 1);
selectorOptions.options.baseLayers.unshift(removed[0]!);Also, I did not implement the opacity slider. I will wait until we have to upgrade the WFRC project that uses it. |
|
Alright, let's make some time to look through this together. Yeah, the randomizer makes sense. We could shuffle the array we pass to the options as well. The only bummer is that the base map order will always be different. Hopefully that's not too weird for users. I think the opacity thing was smart to omit. I remember you were surprised we merged it in as is. There is most likely a better solution. I'm thinking an api like tanstack table where you can enhance the list with legend and opacity features when you need them. |
8305a5f to
808a7b9
Compare
…rite using tailwind and design system components
It's already run in lint job
Here's the error message that prettier was giving on the duration class: warn - The class `duration-[525ms]` is ambiguous and matches multiple utilities. warn - If this is content and not a class, replace it with `duration-[525ms]` to silence this warning.
995a403 to
a31a52e
Compare
I started by branching off of your branch but I must have rebased because if I target that branch in the PR, there are tons of commits and changes.
A lot of work on eslint v8 -> v9 plus refining the config to make it work with typescript. This may be a candidate for updating https://github.com/agrc/eslint-config.
Fixed some typescript config issues that were hiding some errors.
Added prettier --check rather than running prettier as part of eslint. I saw this as a recommendation in someone's docs.
Wired up new layer selector to map and layers. Still not working with the WFRC layer in the stories for some reason.
Maybe we can pair on this next week and get it over the finish line?
Also, did we decide that this should be folded into the design system package?
Also, I wonder if there is opportunity to simply the api for this component with a breaking change. I'm thinking about passing props to the layer constructor specifically.