This repository has been archived by the owner on Dec 11, 2019. It is now read-only.
Updated General Preferences #4640
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
yeah. thanks! |
|
rebased and ready for review 😄 |
|
Look after review w/ Brad
Search screen was tweaked also:
|
|
great job here, looks good to me ++ (+1 for disabling "show home button.." when field is empty) |
|
👍 Code looks great. Is there any reason to keep those three strings in bookmarksToolbarMode.js as opposed to just inlining them? |
|
@jkup yes- there are a few other places it's used (main.js for example, for determining if whethere or not to show favicon). It's nice to be able to do a require and then use the constant like an enum (in case we change the underlying value) 😄 |
- Show home button now disables itself when home page is cleared (null/empty) - Removed appearance settings; moved to under "My home page is" per the mockup on #2108 - Bookmarks toolbar switches were changed into a single drop down Unit tests are in place and working + I've manually at least two of the scenarios outlined in the attached steps Fixes #2108 Fixes #3008 Fixes #2111 Possibly fixes #3544 Auditors: @BrendanEich - you had previously reviewed the settings code; LMK how the changes in `./js/settings.js` look @bbondy - small changes EXCEPT the new config value for BTB. This only had two touchpoints (main.js, contextMenus.js) versus the password manager changes Tweaks made per review w/ Brad. Includes: - reducing space between label and field in preferences - the lone toggle switch was pulled over to the left - import button is moved up - import button area is no longer a separate grouping Upgrade test plan: 1. Install a previous version of Brave (0.12.4 or older); ENABLE favicons and ENABLE show only favicons 2. Upgrade to this version (0.12.5?). Confirm it consolidates preference to "Favicons only" in preferences drop down 3. Install a previous version of Brave (0.12.4 or older); ENABLE favicons but DISABLE show only favicons 4. Upgrade to this version (0.12.5?). Confirm it consolidates preference to "Text and Favicons" in preferences drop down 5. Install a previous version of Brave (0.12.4 or older); DISABLE favicons and DISABLE show only favicons 6. Upgrade to this version (0.12.5?). Confirm it consolidates preference to "Text only" in preferences drop down 7. Uninstall Brave completely; move session file out of the way Regular test plan: ** consolidation of bookmarks toolbar settings** 1. Install a clean version of Brave (0.12.5?) 2. Launch preferences, confirm preference defaults to "Text only" 3. Confirm no favicons show (only text): - at the root level of the bookmarks toolbar - when expanding a folder inside the bookmarks toolbar 4. Repeat step 2/3 for the other two settings. NOTE: show only favicon does not apply to items inside bookmark folders ** bookmark toolbar enable/disable change ** 5. Go to preferences and toggle "Always show the bookmarks toolbar" 6. If disabled, notice how drop down is now disabled too (not allowing you to change) 7. When enabled, notice how drop down is re-enabled ** disabling of show home button ** 8. Set your home page to brave.com and enable "show home button" 9. Erase out the value in home page and confirm "show home button" is: - is turned off - disabled completely (not clickable) 10. Type a non-whitespace character or characters into home page box 11. Confirm the "show home button" is clickable again ** overall general settings tab ** 12. Ensure screen matches corresponding parts of mockup shown in #2108 13. Ensure screen matches corresponding parts of mockup shown in #2111
|
Rebased to fix conflict. I'll go ahead and merge this one, making sure to leave #3544 open as no feedback was received regarding those changes |
This was referenced Oct 20, 2016
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
git rebase -ito squash commits (if needed).Updated General Preferences
Unit tests are in place and working + I've manually at least two of the scenarios outlined in the attached steps
Fixes #2108
Fixes #3008
Fixes #2111
Possibly fixes #3544 (auditor discretion)
Auditors
@bbondy, @ayumi, @cezaraugusto - small changes EXCEPT the new config value for BTB. This only had two touchpoints (main.js, contextMenus.js) versus the password manager changes
@BrendanEich - you had previously reviewed the settings code; LMK how the changes in
./js/settings.jslookUpgrade test plan
Regular test plan
consolidation of bookmarks toolbar settings
bookmark toolbar enable/disable change
disabling of show home button
overall general settings tab
Screenshots
cc: @bradleyrichter
Before
After
