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

Polish groupedItem button & add groupedItem to the existing browserButtons #9357

Merged
merged 2 commits into from
Jun 19, 2017
Merged

Polish groupedItem button & add groupedItem to the existing browserButtons #9357

merged 2 commits into from
Jun 19, 2017

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Jun 9, 2017 •
edited
Loading

Fixes #9251
Fixes #9252

Test Plan:

  1. Open about:styles
  2. Click buttons
  3. Make sure the buttons with 'groupedItem' have margin-left only

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

@luixxiul luixxiul added this to the 0.18.x milestone Jun 9, 2017
@luixxiul luixxiul self-assigned this Jun 9, 2017
luixxiul
luixxiul commented Jun 9, 2017
View reviewed changes
app/renderer/components/common/browserButton.js Outdated
// 'mm' assures theoretically the equal width of margin
// on every platform with any display monitor resolution.
// 2 mm = 1/12 inch
marginLeft: '2mm !important',
Copy link
Contributor Author

@luixxiul luixxiul Jun 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

the value might be changed to a better one. CC @bradleyrichter for ergonomic insight.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a strong guess that most monitors aren't yet in the same high-res as printers to get absolute units with the same precision. Absolute units are basically made for printed pages.

If you're interested, please check https://www.w3.org/Style/Examples/007/units.en.html and lmk if that changed your mind or, if not, lmk your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

otherwise lgtm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolute units are basically made for printed pages.

yes I think you are right. I'll take a look at the document later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with b649105

@alexwykoff alexwykoff modified the milestones: 0.19.x, 0.18.x (Frozen, only critical adds from here) Jun 12, 2017
@luixxiul luixxiul removed the request for review from cezaraugusto June 15, 2017 09:02
@cezaraugusto cezaraugusto mentioned this pull request Jun 15, 2017
Merged
8 tasks
Suguru Hirahara added 2 commits June 18, 2017 16:18
Polish groupedItem button
3dfc598 
Fixes #9251
Fixes #9252

Auditors:

Test Plan:
1. Open about:styles
2. Click "buttons"
3. Make sure the buttons with 'groupedItem' have margin-left only
Add groupedItem to the existing browserButtons
5a46e49
@luixxiul
Copy link
Contributor Author

The comment was addressed and the commits were updated :-)

cezaraugusto
cezaraugusto approved these changes Jun 19, 2017
View reviewed changes
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.

+++++++

@cezaraugusto cezaraugusto merged commit 67033fd into brave:master Jun 19, 2017
@cezaraugusto cezaraugusto deleted the polish-groupedItem branch June 19, 2017 01:15
@luixxiul luixxiul mentioned this pull request Jun 19, 2017
Closed
@luixxiul luixxiul mentioned this pull request Aug 28, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cezaraugusto cezaraugusto cezaraugusto approved these changes

Assignees

@luixxiul luixxiul

Labels
misc/button
Projects
None yet
Milestone
0.19.x (Beta Channel)
Development

Successfully merging this pull request may close these issues.
3 participants
@luixxiul @cezaraugusto @alexwykoff