-
Notifications
You must be signed in to change notification settings - Fork 16
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
Create <map-caption> custom element #741
Conversation
I think that the generated aria-label should be removed if the <map-caption> is disconnected / deleted? WDYT? |
This comment was marked as resolved.
This comment was marked as resolved.
This is working great, just had 2 comments:
|
implement <map-caption> custom element for web-map fix minor details and addded comment about potential features and limitations add tests fix minor details for testing Revert "Merge branch 'map-caption' of https://github.com/kevinkim31/Web-Map-Custom-Element into map-caption" This reverts commit 770e2a7, reversing changes made to b9b8ddb. add tests fix minor details for testing add removing function add removing function
I think this would require adding code to mapml-viewer itself, which I'm not sure is an issue or not. |
b2d2b5e
to
6fc0d56
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.
some comments
Fixes:
|
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.
Tests look good, I suggest a few updates, if possible. Thanks!
src/map-caption.js
Outdated
connectedCallback() { | ||
|
||
// calls MutationObserver; needed to observe changes to content between <map-caption> tags and update to aria-label | ||
let mapcaption = this.parentElement.querySelector('map-caption').innerHTML; |
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.
I think you can use this.textContent
here, unless I'm mistaken. Also, the query you're making is returning the first and only the first of potentially many <map-caption>
elements, which will sometimes be this one, and sometimes not.
I think .textContent is appropriate here. I could be convinced otherwise, but See for a discussion: https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent#differences_from_innertext
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 mapcaption, I need this to return the first map-caption only so that I can store it to then compare it to the mapcaption that was removed/modified.
src/map-caption.js
Outdated
// don't change aria-label if one already exists from user (checks when element is first created) | ||
if (!this.parentElement.hasAttribute('aria-label')) | ||
{ | ||
const ariaLabel = this.textContent; |
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.
Good use of textContent, see comment above
src/map-caption.js
Outdated
} | ||
}); | ||
|
||
this.observer.observe(this, { |
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.
Maybe observing too much, perhaps only subtree is necessary?
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.
Still think that we're observing too much for <map-caption>, only need to watch our content / subtree, no? If you disagree, put a comment below, then mark as resolved. Thanks.
src/web-map.js
Outdated
let mapcaption = this.querySelector('map-caption'); | ||
|
||
if (mapcaption != null){ | ||
setTimeout(() => { |
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.
Why is it required to put this on the event queue?
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.
This is so that the ariaupdate has time to load
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.
A couple of small things. A test for when you add <map-caption> inside e.g. <layer-> to show it doesn't add aria-label to that element would be good.
src/map-caption.js
Outdated
} | ||
}); | ||
|
||
this.observer.observe(this, { |
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.
Still think that we're observing too much for <map-caption>, only need to watch our content / subtree, no? If you disagree, put a comment below, then mark as resolved. Thanks.
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.
LGTM
Closes #736