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

Refactor notificationBar & updateBar #7831

Merged
merged 1 commit into from
Mar 23, 2017
Merged

Refactor notificationBar & updateBar #7831

merged 1 commit into from
Mar 23, 2017

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Mar 22, 2017

Refactor notificationBar & updateBar (including the ledger notification bar which was implemented by @ayumi with PR #3856 of the issue #3257) with aphrodite.

From:
screenshot 2017-03-23 14 39 49

To:
screenshot 2017-03-23 16 30 08

Closes #7853

Auditors:

Test Plan:

  1. Close the browser
  2. Edit <UpdateNotAvailable> to <UpdateAvailable> on updateBar.js
  3. Edit ledger-state.json to display the ledger notification bar
  4. Open the browser
  5. Check for update...
  6. Open https://browserleaks.com/geo
  7. Wait until the ledger notification bar appears
  • 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:

Closes #7853

Instead of applying float:right to place the buttons (span className='options') to the right, separate the buttons wrapper and the other elements, and place them with display:flex and justify-content:space-between (styles.flexJustifyBetween) . Fix #6749

Remove updateBar.less, adding notificationBar__greetingStyle to commonStyles.js

Unify the className 'primaryButton'
  - Renamed 'buttonPrimary' to 'primaryButton' on commonStyles.js and app/renderer/components/preferences/payment/*.js
  - Renamed 'primary' to 'primaryButton' on app/ledger.js

Add styles to commonStyles.js
  - notificationBar
  - notificationBar__notificationItem
    - Introduce border-box
  - notificationBar__greetingStyle
  - notificationItem__greeting
  - notificationItem__message
  - notificationItem__secondaryMessage
  - notificationItem__button

Add a style to global.js
  - 'tabsToolbarBorderColor' (unify the border color of the buttons and the tabs toolbar)
  - 'navbarMenubarMargin'

Replace 'button' with 'css(commonStyles.notificationItem__button)'
TODO: Refactor button.less to remove 'browserButton', 'whiteButton', and 'primaryButton' from notificationBar.js and updateBar.js

Change data-test-id='options' to data-test-id='notificationOptions' on notificationBar.js

Add data-test-ids to updateBar.js
  - data-test-id='updateHide'
  - data-test-id='updateDetails'
  - data-test-id='updateLater'
  - data-test-id='updateNow'

Reduce button's line-height to 1

Auditors:

Test Plan:
1. Close the browser
2. Edit <UpdateNotAvailable> to <UpdateAvailable> on updateBar.js
3. Edit ledger-state.json to display the ledger notification bar
4. Open the browser
5. Check for update...
6. Open https://browserleaks.com/geo
7. Wait until the ledger notification bar appears
@luixxiul luixxiul changed the title WIP: refactor notificationBar & updateBar Refactor notificationBar & updateBar Mar 23, 2017
@luixxiul luixxiul self-assigned this Mar 23, 2017
@luixxiul luixxiul requested a review from bradleyrichter March 23, 2017 08:08
@luixxiul
Copy link
Contributor Author

I missed that #6939 was under WIP.

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Didn't try it, but changes LGTM; will merge for next Preview / RC

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