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

clean up element styling #5442

Merged
merged 1 commit into from
Nov 15, 2016
Merged

clean up element styling #5442

merged 1 commit into from
Nov 15, 2016

Conversation

jkup
Copy link
Contributor

@jkup jkup commented Nov 7, 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).

Auditors: @bsclifton @bradleyrichter

Fix #5095 #5094

Testing:

  1. Check all Preferences pages to make sure no form styles are broken. The input and select fields should look identical to master.
  2. Go to about:styles and make sure the documentation and UI is acceptable for shipping.

screen shot 2016-11-14 at 11 38 47 am

@jkup
Copy link
Contributor Author

jkup commented Nov 7, 2016

@bsclifton my fault! I think I wasn't clear, I just wanted to put up a WiP to get your opinion on whether the adding of the class should be done manually or hidden away in a separate type component?

@jkup jkup added this to the 0.12.10 release milestone Nov 8, 2016
@jkup jkup force-pushed the element-styling branch 4 times, most recently from 3cb6463 to 532e29a Compare November 14, 2016 16:37
@jkup jkup changed the title [WiP] clean up element styling clean up element styling Nov 14, 2016
@bsclifton
Copy link
Member

Deleted my above comments from when this was a WIP 😄

Preferences still look great. When going to about:styles, it seems like the text and select inputs both are smaller than on the preferences page (wasn't sure if this was expected).

One consideration (curious what both you and @bradleyrichter think) would be changing styles on focus. One usability problem in Brave is that using keyboard navigation is near impossible in certain scenarios (like the preferences page). It would be great to have a visual indicator of where the tab focus currently is

@bsclifton
Copy link
Member

From comparing to other parts of the app, it looks like the White app button is missing it's hover style; if you go to Preferences > Payments, you can see a demo there which I believe is @buttonColor and then #000 on hover

@bsclifton
Copy link
Member

bsclifton commented Nov 14, 2016

Another consideration too might be to put:
cursor: default and -webkit-user-select: none on some of the elements, like the h1, h2, etc. Since we'd want to make it more app-like.

@bsclifton
Copy link
Member

Looks great! 😄 (comments left above)

@jkup
Copy link
Contributor Author

jkup commented Nov 14, 2016

Comments sound good:

  1. They do look a bit smaller. I'll figure out where that size or padding is coming from and fix it.
  2. I would very much like to add focus styles on our form elements. It's nice for all users but a huge accessibility win as well.
  3. I see the white button as having the right colors: Implement history and Ctrl +Y to bring it up #444 regularly and #000 on hover.
  4. Happy to add cursors to the headers! Let me know what we should do!

@jkup
Copy link
Contributor Author

jkup commented Nov 15, 2016

@bsclifton I fixed the sizing thing! Should we merge this for now and sync with @bradleyrichter later about cursors and focus state?

@bradleyrichter
Copy link
Contributor

Does this include the dream (mine) of having all buttons now connected so I can refine the styles across the entire app? Or is this just the first step to that dream?

@bsclifton
Copy link
Member

@bradleyrichter this should be that dream! 😄 What do do you think about the app cursor comments I had above?

@bsclifton
Copy link
Member

Merging 😄 If we need any changes, let's create a new issue

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. QA/checked-Linux QA/checked-macOS QA/checked-Win32 QA/checked-Win64 refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants