-
Notifications
You must be signed in to change notification settings - Fork 971
Refactor about:passwords with Aphrodite #7336
Conversation
js/about/passwords.js
Outdated
return <PasswordsTr data-test-id='passwordItem'> | ||
<ActionsTd> | ||
<span className={css(styles.passwordAction)}> | ||
<span className='fa fa-times' title='Remove site' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we go ahead on this and localize title? change it to data-l10n-id='removeSite'
and then on passwords.properties
add
removeSite.title=Remove site
Also add fa fa-times
icon to global.js
under appIcons
property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we go ahead on this and localize title?
I noticed but did not fix as I thought it was another issue. It should be handled with a follow up task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np either way is fine
js/about/passwords.js
Outdated
return <PasswordsTr data-test-id='passwordItem'> | ||
<ActionsTd data-test-id='passwordActions'> | ||
<span className={css(styles.passwordAction)}> | ||
<span className='fa fa-times' title='Delete password' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto for icon class
js/about/passwords.js
Outdated
</PasswordsTd> | ||
<ActionsTd data-test-id='passwordActions'> | ||
<span className={css(styles.passwordAction)}> | ||
<span className='fa fa-clipboard' title='Copy password to clipboard' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as first comment regarding icon and title
cursor: pointer; | ||
text-decoration: underline; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super cool to see LESS files removed ++
@@ -116,7 +116,7 @@ describe('notificationBar', function () { | |||
}).click('button=Yes') | |||
.tabByIndex(0) | |||
.loadUrl('about:passwords') | |||
.waitForExist('tr.passwordItem') | |||
.waitForExist('.passwordItem') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be .waitForExist('[data-test-id="passwordItem"]')
You can check if it works by running npm run watch-test
and on other terminal tab run
npm run test -- --grep="autofills remembered password on login form"
Closes #7335 - passwords.less is removed - PasswordsTh, PasswordsTr (HeadTr), and PasswordsTd (ActionsTd) are introduced - 'fa fa-times' is added to appIcons as 'remove' - appIcons are alphabetized Auditors: Test Plan: 1. Remember a password by logging in GitHub 2. Open about:passwords 3. Make sure styling is not broken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes #7335
Auditors:
Test Plan:
git rebase -i
to squash commits (if needed).