-
Notifications
You must be signed in to change notification settings - Fork 420
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
Popup menu cleanup #697
Popup menu cleanup #697
Conversation
aa81b99
to
c2d9344
Compare
onKeyup=${ handleKey } | ||
onKeydown=${ handleKeyDown } | ||
/> | ||
${entry.imageUrl ? html`<img src=${ entry.imageUrl } />` : null} |
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.
One thing that I found is that we have to be careful with the way we format html template strings. Best practice is to have start and end on separate, individual lines.
Should we not have inline html`` like here then?
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.
It does not screw up the markup, hence I'd be OK with this snippet.
What breaks the markup entirely is stuff like this:
html`
<div>
<a>...</a>
</div>`
=> Asymetric use of tagged template literal open + close.
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.
Got it!
Description says component but I guess you meant in the commit description since you moved from the Component to the List |
16b0616
to
054ce1f
Compare
Fixed #697 (comment) via 05ddb4f. |
Scrolling into view can be handled within the <PopupMenuList />
* drop bogus `shouldOpen` handling not available in original menu * use `PopupMenu#trigger` to activate entries
054ce1f
to
fa7b346
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.
Looks good 👍
Proposing this PR to address #696 (comment) + minor nit picks (mainly around formatting).
One thing that I found is that we have to be careful with the way we format
html
template strings. Best practice is to have start and end on separate, individual lines:If we approach it like this then we end up with very readable templates, and eslint is happy, too.