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

Restyled select and input to the spec image #6084

Closed
wants to merge 9 commits into from
Closed

Restyled select and input to the spec image #6084

wants to merge 9 commits into from

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Dec 8, 2016

  • 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).

Closes #5972
The SVG caret down will be replaced with the sprite later (#5891)

Test Plan: in fd0c10a

Auditors:

screenshot 2016-12-08 14 53 33

Into
screenshot 2016-12-11 19 01 19
screenshot 2016-12-08 15 36 51

It seems there is no property such as outline-radius.

screenshot 2016-12-08 15 44 19

@luixxiul luixxiul added design A design change, especially one which needs input from the design team. settings labels Dec 8, 2016
@luixxiul
Copy link
Contributor Author

luixxiul commented Dec 8, 2016

If there is (and should be) a better way to handle radius inside the input form, please add edits to the PR, thanks.

@bradleyrichter
Copy link
Contributor

sweet!

I do see that we need one more style difference between editable text entry fields and non-editable. (can be the same but light grey BG instead of white.)

@luixxiul
Copy link
Contributor Author

luixxiul commented Dec 8, 2016

@bradleyrichter even @veryLightGray in variable.less is too dark on about:preferences, though I'm not sure if #fefefe makes difference.

@luixxiul luixxiul added the polish Nice to have — usually related to front-end/visual tasks. label Dec 8, 2016
@bsclifton bsclifton assigned bsclifton and unassigned bsclifton Dec 9, 2016
@bsclifton bsclifton added this to the 0.13.1 milestone Dec 9, 2016
@luixxiul
Copy link
Contributor Author

be2a185 fixes

screenshot 2016-12-10 16 51 33

into
screenshot 2016-12-10 16 51 40

@luixxiul
Copy link
Contributor Author

With 21a13ef

screenshot 2016-12-10 17 24 44

@@ -22,7 +22,8 @@ select {
width: 100%;
}

select.form-control {
select.form-control,
.bookmarkForm select {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know this is not an elegant solution; general properties for select and input should be specified at first, followed by more specific properties for .form-control, etc.

@luixxiul
Copy link
Contributor Author

luixxiul commented Dec 11, 2016

With f21ff68

screenshot 2016-12-11 18 55 55

the length of the input form on about:preferences#payments is fixed after #6047 is merged like this:
screenshot 2016-12-11 19 01 19

@luixxiul luixxiul changed the title Restyled select.form-control and input.form-control to the spec image Restyled select and input to the spec image Dec 11, 2016
@bradleyrichter
Copy link
Contributor

@luixxiul These look good. The one thing I see that is still wrong is that the form elements in the bookmark hanger should be taller to match the prefs pages.

@luixxiul
Copy link
Contributor Author

@bradleyrichter Should it be like this?

screenshot 2016-12-12 4 42 55

@bradleyrichter
Copy link
Contributor

bradleyrichter commented Dec 11, 2016

yes. Now they match. But perhaps we can tighten up the padding across all of them now?

About this much: (see thinner vs current on both objects)

image

@luixxiul
Copy link
Contributor Author

luixxiul commented Dec 12, 2016

OK, updated. ready for re-review.

@bsclifton
Copy link
Member

Will check this out soon (I promise!)

@bbondy bbondy modified the milestones: 0.13.1, 0.13.0 Dec 19, 2016
@bradleyrichter
Copy link
Contributor

@bsclifton Candidate for 13.0? Or wait?

@bsclifton
Copy link
Member

Let's keep this for 0.13.1 😄

maazadeeb and others added 4 commits January 9, 2017 02:21
Majorly, the change affects 2 places in the UI. The Clear Browsing
Data… option in the History menu and the Clear Browsing Data Now…
button in the Security tab in Preferences. We store the state of the
toggled options when the user clears anytime, so that the next time
he/she opens this dialog, the options are pre-populated as per the last
selected options

From a technical perspective, we have moved away from saving the
current state of the SwitchControls in the windowState. We only save
the visibility on the windowState. The whole state is managed by the
panel itself and just updates the appState on clicking clear.

Fixes #4082
Making the Clear Browsing Data panel stateful
@luixxiul luixxiul changed the base branch from master to 0.13.1-branch January 11, 2017 06:59
Add focus ring to about page form elements
Add Paste Without Formatting to context menu
Closes #5972

- Set -webkit-appearance:none
- Set the caret down SVG file (created by @bradleyrichter) as the background-image
- Set the outline inside the input element
- Set height:2rem to make the height consistent
- Moved properties in .form-control to the input and select element
- Removed box-shadow properties from the input and select element under .bookmarkForm
- Set specific properties to input in #navigator

The caret down SVG file will be replaced with the sprite PNG later (See #5891)

Auditors:

Test Plan:
1. Open about:preferences
2. Make sure the text entry fields with active edit state have the rectangle inside them
3. Make sure the drop down arrows are styled properly
4. Open another page in a new tab
5. Open the shield from the top right lion icon
6. Make sure the drop down arrows are styled properly
7. Bookmark about:preferences
8. Make sure the input and select elements are styled consistently on the bookmark dialog
@luixxiul
Copy link
Contributor Author

@cezaraugusto
Copy link
Contributor

cezaraugusto commented Jan 15, 2017

looks good but needs rebase

@bsclifton
Copy link
Member

bsclifton commented Jan 16, 2017

I've been very hesitant to review this PR because of all of the time cost associated with bugs and regressions that style updates introduce.

Per our reviewer guidelines, I'm going to close this PR since it doesn't meet our requirements:
https://github.com/brave/browser-laptop/wiki/Reviewer-guideline

  1. Front end: If styles are being added, make sure it is using Aphrodite.

I'll update the original issue to capture this requirement

@bsclifton bsclifton closed this Jan 16, 2017
@luixxiul luixxiul deleted the fix-input-select-styles branch January 16, 2017 03:10
@luixxiul luixxiul removed this from the 0.13.1 milestone Jan 16, 2017
@luixxiul
Copy link
Contributor Author

�OK but please make sure that the guideline was published 8 days ago while the PR was initially opened 1 month ago

@bsclifton
Copy link
Member

@luixxiul I understand. I definitely appreciate the work, but the cost of reviewing adhoc style changes is orders of magnitude higher than reviewing code which has tests.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
design A design change, especially one which needs input from the design team. polish Nice to have — usually related to front-end/visual tasks. settings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants