-
Notifications
You must be signed in to change notification settings - Fork 974
Update siteInfo.js to the modified BEM style #10933
Conversation
Closes #10932 Auditors: Test Plan: 1. Open https://mixed-script.badssl.com/ 2. Open https://mixed.badssl.com/ in a new tab 3. Open http://http.badssl.com/ in a new tab 4. Click the lock icon of each tab 5. Make sure the site info dialogs are properly displayed
[css(styles.siteInfo__wrapper__large)]: (this.props.isBlockedRunInsecureContent || this.props.runInsecureContent) | ||
})}> | ||
className={css( | ||
commonStyles.flyoutDialog, |
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.
Creating <FlyoutDialog>
to remove commonStyles.flyoutDialog.
eg.
<Dialog ...>
<FlyoutDialog custom={
styles.siteInfo,
this.props.isBlockedRunInsecureContent && styles.siteInfo_large
}>
...
</FlyoutDialog>
</Dialog>
Almost done. See: #10977
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.
there's a PR for that from you (#10978) should it land first then this?
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 is ok to merge this first.
<span className={cx({ | ||
fa: true, | ||
'fa-unlock': true, | ||
[globalStyles.appIcons.unlock]: true, |
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.
Wondering if we would create components like <AppIcon unlock custom={styles.secureIcon__fa} />
to replace span and fa classes with cx
with normalized components.
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.
I would prefer having the icons instead of a FA class. Food for thought if you want to go ahead with this we could gradually remove them in favor of SVG. Sources can be found here: https://github.com/encharm/Font-Awesome-SVG-PNG
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.
++
Closes #10932
Auditors:
Test Plan:
Submitter Checklist:
git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
Tests