Skip to content
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

chore: refactor eox-map and convert code from ts to js #1206

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

srijitcoder
Copy link
Collaborator

@srijitcoder srijitcoder commented Aug 9, 2024

Implemented changes

  • refactoring from TS to JS + JSDoc
  • splitting up stories
  • methods/helpers etc. analog to the other elements
  • no breaking changes (properties, API etc.)
  • include typechecking from the start
  • expose layer config (and other configs) in dedicated files/types and to the docs (maybe there is a storybook plugin to render certain types? or rendering certain lines)
  • Test will be done separately once the first eox-map refactored version is released.

Screenshots/Videos

Checklist before requesting a review

Copy link

netlify bot commented Aug 9, 2024

Deploy Preview for eoxelements ready!

Name Link
🔨 Latest commit c38fba9
🔍 Latest deploy log https://app.netlify.com/sites/eoxelements/deploys/66f5738b02494a0009c8a4a0
😎 Deploy Preview https://deploy-preview-1206--eoxelements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@srijitcoder srijitcoder linked an issue Sep 19, 2024 that may be closed by this pull request
4 tasks
@srijitcoder srijitcoder marked this pull request as ready for review September 25, 2024 11:06
Copy link
Collaborator

@silvester-pari silvester-pari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a huge undertaking, thank you so much for this! On the first glance and in first integration tests it seems to work properly, but even if there should be any issues we can fix them at a later stage...

Copy link
Contributor

@RobertOrthofer RobertOrthofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also from my side i have no real complaints, great work!

This is a ginormous PR (as expected), I only had a quick glance but it looks good to me.

Comment on lines +106 to +115
if (newControls) {
const keys = /** @type {ControlType[]} **/ Object.keys(controls);
for (let i = 0, ii = keys.length; i < ii; i++) {
const key = /** @type {ControlType} **/ keys[i];

// Add or update the control using the helper function
// @ts-expect-error - Error throwing even type is declared.
addOrUpdateControl(EOxMap, oldControls, key, controls[key]);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, the only thing missing here are the round parenthesis ( ) around the variable to perform a proper type cast.

Suggested change
if (newControls) {
const keys = /** @type {ControlType[]} **/ Object.keys(controls);
for (let i = 0, ii = keys.length; i < ii; i++) {
const key = /** @type {ControlType} **/ keys[i];
// Add or update the control using the helper function
// @ts-expect-error - Error throwing even type is declared.
addOrUpdateControl(EOxMap, oldControls, key, controls[key]);
}
}
if (newControls) {
const keys = /** @type {ControlType[]} **/ (Object.keys(controls));
for (let i = 0, ii = keys.length; i < ii; i++) {
const key = keys[i];
// Add or update the control using the helper function
addOrUpdateControl(EOxMap, oldControls, key, controls[key]);
}
}

@@ -114,7 +141,7 @@ export class EOxMapCompare extends TemplateElement {
() => html`
<div class="eox-map-compare">
<div class="eox-map-compare__first">
<slot name="first"></slot>
<slot name="first">hujhjjh</slot>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor typo, it's spelled Juhuuu! (Hooray!) in German ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eox-map refactoring
3 participants