-
Notifications
You must be signed in to change notification settings - Fork 973
Conversation
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 left a comment asking for a minor change and otherwise LGTM
app/renderer/components/list.js
Outdated
this.props['data-isDownload'] && styles.isDownload | ||
) | ||
|
||
return <list className={className} {...this.props} /> |
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 don't think <list>
is a valid element. Lists are tagged as grouping content and AFAICT this tag is not included as one. Can we replace that with just a div
? btw ok to keep component name as List
.
Ref: https://www.w3.org/community/webed/wiki/HTML/Elements#Grouping_content
overflow: 'hidden', | ||
whiteSpace: 'nowrap', | ||
padding: '8px 12px', | ||
WebkitUserSelect: 'none', |
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.props['data-isDownload'] && styles.isDownload | ||
) | ||
|
||
return <div className={className} {...this.props} /> |
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.
@cezaraugusto updated.
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
including:
<List>
and<DownloadList>
in list.js&.selected
Closes #7255
Addresses #7318
Auditors: @bsclifton @cezaraugusto
Test Plan:
git rebase -i
to squash commits (if needed).Test Plan: