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

Polish notification button #9239

Merged
merged 3 commits into from
Jun 9, 2017
Merged

Polish notification button #9239

merged 3 commits into from
Jun 9, 2017

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Jun 3, 2017

Fixes #9237
Fixes #9238
Closes #9280

Test Plan 1

  1. Open about:preferences#general
  2. Change lang setting
  3. Click No with a mouse using an arm opposite to a dominant arm
  4. Check if you could click the button more easily than the current one

Test Plan 2

  1. Open about:styles#buttons
  2. Make sure the notificationItem button is rendered as a white button
  3. Make sure the code samples are displayed inside scrollable wrappers

Submitter Checklist:

  • 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).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@@ -88,7 +88,7 @@ const styles = StyleSheet.create({
cursor: 'pointer',
lineHeight: 1.25,
width: 'auto',
height: 'auto',
height: '32px',
Copy link
Contributor Author

@luixxiul luixxiul Jun 4, 2017

Choose a reason for hiding this comment

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

The reason why setting this value is to force the specified height. By setting it to auto the value can slightly (~2px) change as it is not intended.

Known issue: with this change the buttons on Brave payments gets broken. Setting their height to 37px would fix the issue.

Copy link
Contributor Author

@luixxiul luixxiul Jun 4, 2017

Choose a reason for hiding this comment

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

Otherwise: simply removing the font-size and padding specification on button.less makes the buttons size standard.

If the buttons become too small due to the change, then we would have to create another component like preferenceItem.

Copy link
Contributor

@bradleyrichter bradleyrichter left a comment

Choose a reason for hiding this comment

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

looks good to me. thanks....

@cezaraugusto
Copy link
Contributor

needs rebase pls

from a viewpoint of accessibility

Fixes #9237

Auditors:

Test Plan:
1. Open about:preferences#general
2. Change lang setting
3. Click 'No' with a mouse using an arm opposite to a dominant arm
4. Check if you could click the button more easily than the current one
@luixxiul luixxiul requested review from cezaraugusto and removed request for cezaraugusto June 6, 2017 03:16
@luixxiul luixxiul removed the request for review from cezaraugusto June 6, 2017 05:50
Suguru Hirahara added 2 commits June 6, 2017 16:53
Fixes #9238
Closes #9280

- Reorganize secondaryColor and primaryColor buttons
- Add overflow-x to pre
- Add secondaryColor to notificationItem

Auditors:

Test Plan:
1. Open about:styles#buttons
2. Make sure the notificationItem button is rendered as a white button
3. Make sure the code sample boxes are scrollable
@cezaraugusto
Copy link
Contributor

Click No with a mouse using an arm opposite to a dominant arm

I almost broke my arm doing this 😝

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.

++

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.

3 participants