Changed style on tool tip to match other tool tips in section setup #54865
Conversation
|
|
||
| export default function InfoHelpTip({id, content}) { | ||
| return ( | ||
| <div> | ||
| <span style={{marginLeft: '12px'}}> |
There was a problem hiding this comment.
I know this is nit-picky, but from a code style perspective, would we rather have all of this CSS in the styles variable at the end of of file? Since this was only one style change, I felt like it was overkill to add it to the styles variable - it seemed to make it less readable to me, but I'd love to know the hive's thoughts here.
There was a problem hiding this comment.
I think in general we prefer to use .module.scss when there is not already a 'styles' block in the component, or try to convert the component to .module.scss.
However, in this case, since the class already has styles and we are trying to move quickly, I don't think we should convert.
I think both are fine. A couple data points: if we do convert the styles to css, then we may miss the inline style because it isn't in the same block. Additionally, I think giving the span a nice name shows the reader what the span actually does, rather than just the styles we are applying to it. But the inline is shorter and clearer to tell what styles are applied so shrug
|
|
||
| export default function InfoHelpTip({id, content}) { | ||
| return ( | ||
| <div> | ||
| <span style={{marginLeft: '12px'}}> |
There was a problem hiding this comment.
I think in general we prefer to use .module.scss when there is not already a 'styles' block in the component, or try to convert the component to .module.scss.
However, in this case, since the class already has styles and we are trying to move quickly, I don't think we should convert.
I think both are fine. A couple data points: if we do convert the styles to css, then we may miss the inline style because it isn't in the same block. Additionally, I think giving the span a nice name shows the reader what the span actually does, rather than just the styles we are applying to it. But the inline is shorter and clearer to tell what styles are applied so shrug
apps/src/lib/ui/InfoHelpTip.jsx
Outdated
| infoToolTipBox: { | ||
| maxWidth: '400px', | ||
| whiteSpace: 'normal', | ||
| fontFamily: '"Metropolis", sans-serif', |
There was a problem hiding this comment.
When do we need to set this explicitly? Is it not set globally to Metropolis after the migration?
There was a problem hiding this comment.
You're right! I will remove it.
apps/src/lib/ui/InfoHelpTip.jsx
Outdated
| infoToolTipText: { | ||
| fontSize: '13px', | ||
| color: 'white', | ||
| marginBottom: '0em', |
There was a problem hiding this comment.
nit:
| marginBottom: '0em', | |
| marginBottom: '0', |
apps/src/lib/ui/InfoHelpTip.jsx
Outdated
| }, | ||
| infoToolTipText: { | ||
| fontSize: '13px', | ||
| color: 'white', |
There was a problem hiding this comment.
nit: I think we still want to use @cdo/apps/util/color
| return renderExpandableSection( | ||
| 'uitest-expandable-coteacher', | ||
| () => ( | ||
| <div> | ||
| {i18n.coteacherAdd()} | ||
| {tooltip} | ||
| <InfoHelpTip |
This ensures the tool tips from the Co-teacher section match the styles of the tips in Advanced Settings.
New styles:
Matching the Advanced Setting Styles:
Links
Ticket
Testing story
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Checklist: