Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Refactor download.js with Aphrodite #7258

Merged
merged 1 commit into from
Feb 23, 2017
Merged

Refactor download.js with Aphrodite #7258

merged 1 commit into from
Feb 23, 2017

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Feb 15, 2017

including:

  • Created <List> and <DownloadList> in list.js
  • Copied itemList.less to commonStyles.js except &.selected
  • Added aboutPageSectionMargin to global.js

Closes #7255
Addresses #7318

Auditors: @bsclifton @cezaraugusto

Test Plan:

  1. Open about:download
  2. Download a file
  3. Make sure the styles are not broken
  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Test Plan:

Copy link
Contributor

@cezaraugusto cezaraugusto left a 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

this.props['data-isDownload'] && styles.isDownload
)

return <list className={className} {...this.props} />
Copy link
Contributor

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',
Copy link
Contributor

Choose a reason for hiding this comment

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

++

including:

- Created <List> and <DownloadList> in list.js
- Copied itemList.less to commonStyles.js except '&.selected'
- Added aboutPageSectionMargin to global.js

Closes #7255
Addresses #7318

Auditors:

Test Plan:
1. Open about:download
2. Download a file
3. Make sure the styles are not broken
this.props['data-isDownload'] && styles.isDownload
)

return <div className={className} {...this.props} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cezaraugusto updated.

Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

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

LGTM

@luixxiul luixxiul removed the request for review from bsclifton February 23, 2017 06:58
@luixxiul luixxiul merged commit ef121fb into brave:master Feb 23, 2017
@luixxiul luixxiul deleted the aboutDownloads-aphrodite branch February 23, 2017 14:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants