This repository has been archived by the owner on Dec 11, 2019. It is now read-only.
Refactor components with commonForm #8443
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
- textbox and textbox__outlineable were copied from textbox.js to commonStyles.js Since FormTextbox cannot be used for the input elements, I copied the styles applied for that element and applied to them. See: #7164 (comment) The labels and input forms were grouped and placed with display:flex and justify-content:space-between. Also the elements inside each wrapper were aligned equally to make the length of the input forms always equal (l10n friendly). Also colons in the label were removed to make the style consistent. Closes #8009 Addresses #8010 Auditors: Test Plan: 1. Visit http://browserspy.dk/password.php 2. Click "password-ok.php" link 3. Make sure you can log in successfully with the given credential 4. Change the lang setting on about:preferences 5. Try the same steps above and make sure the length of the input forms is equal
Also: - Replaced aphrodite with aphrodite/no-important on textbox.js - Moved the styles from loginRequired.js to commonForm.js Auditors: Test Plan: n/a
Closes #8028 Addresses #7989 - Removed isClickDismiss from the dialog Installing Widevine influences globally, not specific to a tab which opens the page which requires Widevine. Removing isClickDismiss would clarify to the users that the third party software which we discourage from using is going to be installed on the browser right now. - Applied commonForm.js to them in order to make styling consistent - Added CommonFormLarge to commonForm.js - Added globalStyles.dialogWidth and globalStyles.LargeWidth to global.js - Added noMargin and noPadding to commonStyles.js - Added margin-top to the bottom text block on widevineInfo.js - Added testIds to the buttons and the dialog, modifying dialog.js - data-test-id as testId - data-test2-id as test2Id - Added widevinePanelDialog as a testId - Set cursor:pointer Auditors: Test Plan: 1. Visit netflix.com 2. Click outside of the dialog 3. Make sure the dialog is not closed 4. Click "Install and Allow" 5. Log in to the account 6. Make sure movies can be played
Closes #8099 Closes #8100 Addresses #7989 - Refactored with commonForm, replacing it with genericForm Aligned labels to the left and input textboxes to the right by applying display:flex - Added CommonFormLarge - Added cx to dialog.js for more readability - Removed .manageAutofillDataPanel from form.less Also: - Added testIds - Edited autofillTest.js and autofill.js TODO: refactor autofill.js with Aphrodite - Edited selectors.js Auditors: Test Plan: 1. Automated tests should pass 2. Open about:autofill 3. Add an address 4. Add a credit card 5. Visit https://www.roboform.com/filling-test-all-fields 6. Make sure the input forms can be filled
Closes #6040 Addresses #7989 - Added CommonFormBookmarkHanger to commonForm.js - Added globalStyles.spacing.bookmarkHangerMaxWidth to global.js Max-width of the bookmark hanger was set to 350px. Also, setting white-space:normal makes it possible for lines to be wrapped. - Moved Dialog from addEditBookmark.js to addEditBookmarkHanger.js - Added cancel button with this.onClose as 'fa fa-close' was removed - Added const arrowUp and dialogHanger - Added box-shadow to arrowUp - Added styles.bookmarkHanger which is applied if !this.props.isModal - Copied CommonFormSection and CommonFormTitle from commonForm.js, adding CommonFormBookmarkHangerTitle to addEditBookmarkHanger.js The margin-top of the title should be reduced if this.props.isModal. - Changed backgroundColor of commonForm to #fff - Add zindex to elements on navigation bar to let tests pass The value was set to higher than that of dialog, which is 3000. - Updated test code Also: - Replaced divs with sections - Removed styles from form.less - Removed wideButton from buttons - Added testIds to buttons (bookmarkHangerRemoveButton, bookmarkHangerCancelButton, bookmarkHangerDoneButton) - Fixed camel cases Auditors: Test Plan 1: 1. Open about:preferences 2. Disable Home button 3. Click the star icon on the URL bar 4. Make sure the arrow up appears exactly under the star 5. Make sure the button appears under the icon 6. Open about:preferences 7. Enable Home button 8. Click the star icon again 9. Make sure th arrow up appears exactly under the star Test Plan 2: 1. Open about:bookmarks 2. Click the star icon on the URL bar 3. Click "Done" 4. Click the star icon again 5. Click "Remove" 6. Make sure the bookmark is successfully removed Test Plan 3: 1. Click "Add Folder" icon on about:bookmarks 2. Click "Cancel" 3. Make sure the dialog is closed Test Plan 4: 1. Create a bookmark folder on about:bookmarks 2. Edit the folder via context menu on about:bookmarks 3. Click "Remove" 4. Make sure the folder is successfully removed Test Plan 5: 1. Click the star icon on about:bookmarks 2. Click "Cancel" 3. Make sure the dialog is closed 4. Click the star icon again 5. Create a bookmark 6. Edit the bookmark via context menu 7. Click "Remove" 8. Make sure the bookmark is successfully removed
Closes #8192 Addresses #7989 - Added CommonFormSmall to commonForm.js - Added data-test-id and testIds - Removed clearBrowsingDataPanel from forms.less - Updated test code Auditors: Test Plan: 1. Open Clear browsing data panel from the menu 2. Test the cancel button works 3. Reopen the panel 4. Test the clear button works
Auditors: Test Plan: n/a
…o app folder Auditors: Test Plan: n/a
Auditors: Test Plan: n/a
Auditors: Test Plan: n/a
Auditors: Test Plan: n/a
|
If this PR looks good to you, please add |
|
It seems that the errors on the Travis are irrelevant with this PR. |
luixxiul
force-pushed
the
refactoring-aphrodite
branch
from
2b8c60d
to
e7bdb3c
Compare
cezaraugusto
approved these changes
Apr 24, 2017
added 3 commits
April 25, 2017 09:45
Auditors: Test Plan: n/a
Auditors: Test Plan: n/a
Auditors: Test Plan: n/a
luixxiul
force-pushed
the
refactoring-aphrodite
branch
from
1822219
to
16a3171
Compare
Auditors: Test Plan: n/a
This was referenced Apr 25, 2017
Merged
This was referenced Apr 26, 2017
Closed
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.
This PR is based on
refactoring-aphrodite.Refactor components with commonForm.
See:
#8011 - Refactor loginRequired.js with Aphrodite and commonForm
#8032 - Refactor widevinePanel.js and widevineInfo.js
#8105 - Refactor autofillAddressPanel.js and autofillCreditCardPanel.js
#8136 - Refactor addEditBookmarkHanger.js with commonForm
#8154 - Refactor checkDefaultBrowserDialog.js with commonForm
#8155 - Delete genericForm and commonDialog from forms.less
#8194 - Refactor clearBrowsingDataPanel.js with commonForm
Also: move
commonForm.js,list.js,dropdown.js,textbox.jsandmessageBox.jsintocommonfolder.As the PRs above have been reviewed already, there should be no problems with this PR.
Test Plan: described in those PRs
git rebase -ito squash commits (if needed).#6372