-
Notifications
You must be signed in to change notification settings - Fork 16
PB-2064: importtoolmaps cypress test #1514
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
base: feat-PB-1383-pinia-store
Are you sure you want to change the base?
PB-2064: importtoolmaps cypress test #1514
Conversation
7f1a2dd to
2237171
Compare
| layerOrLayerId: WMSCapabilityLayer | string, | ||
| options?: ExternalLayerParsingOptions<ExternalWMSLayer> | ||
| options?: ExternalLayerParsingOptions<ExternalWMSLayer>, | ||
| parentsArray?: WMSCapabilityLayer[], |
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.
could this parentsArray be added to the options bag? would keep the signature cleaner IMHO
| const humanReadableCurrentTimestamp = computed<string>(() => renderHumanReadableTimestamp( | ||
| timeConfig.currentTimeEntry as LayerTimeConfigEntry & { year: string } | ||
| )) |
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.
can you be 100% sure timeConfig.currentTimeEntry is defined in this context? Is there a guard (or v-if) somewhere that checks that
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.
the function renderHumanReadableTimestamp handles an undefined value already, but I will add an ? to the function parameter to make clear that the parameter is optional
| * an "id" constant. | ||
| * | ||
| * @param {any} el Reference to the element | ||
| * @param {Element | ComponentPublicInstance | null} el Reference to the element |
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.
There's no need to type the JSDoc, you can remove all type related definition there as it will be derived from the TS code anyway
| :ref="addRefBySectionId" | ||
| :compact="compact" | ||
| @open-menu-section="onOpenMenuSection" | ||
| @close-menu-section="onCloseMenuSection" |
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.
is that normal that you removed the handler here?
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.
yes it is never called and never used
| title="3D" | ||
| secondary | ||
| @open-menu-section="onOpenMenuSection" | ||
| @close-menu-section="onCloseMenuSection" |
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.
same here, aren't we losing some functionality with that?
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.
same it is never called and never used
| cy.wrap(center[0]).should('be.closeTo', expectedCenter[0], 50) | ||
| cy.wrap(center[1]).should('be.closeTo', expectedCenter[1], 50) |
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.
50 meters out is quite the big leap from 5 meters.
Did it not pass with the 5 meter delta?
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.
no it didn't, i can check again if i can reduce it or why it is 50m off
| return !!EXTERNAL_PROVIDER_WHITELISTED_URL_REGEXES.find( | ||
| (regex) => !!layerBaseUrl.match(regex) | ||
| ) |
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.
| return !!EXTERNAL_PROVIDER_WHITELISTED_URL_REGEXES.find( | |
| (regex) => !!layerBaseUrl.match(regex) | |
| ) | |
| return EXTERNAL_PROVIDER_WHITELISTED_URL_REGEXES.some( | |
| (regex) => !!layerBaseUrl.match(regex) | |
| ) |
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.
For which case did you require that? we aren't running this package "alone" outside of unit tests, so my gut feeling is that we shouldn't need that here
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.
when i didn't have this there, importing certain wmts layers from the import maps tool did not work because certain projections were missing
2237171 to
a34b939
Compare
a1cccff to
9e6d8d7
Compare
8334cc5 to
ded5cad
Compare
bf0add8 to
5505e32
Compare
Test link