Add AudienceSegmentation/InfoNotice #8388
Conversation
|
Build files for c85262b have been deleted. |
|
Size Change: +6.12 kB (0%) Total Size: 1.55 MB
ℹ️ View Unchanged
|
| describe( 'InfoNotice', () => { | ||
| it( 'should render correctly in the default state', () => { | ||
| const { container, getByText } = render( | ||
| <InfoNotice { ...InfoNoticeStory.args } /> |
There was a problem hiding this comment.
This is an interesting approach, but using story definitions in tests like this is not something we've done so far elsewhere, as far as I can see.
Tbh, I'm a bit hesitant on the basis that it makes the test a bit harder to read - it's not entirely clear what it's doing without referring to the story. While DRY is generally a good principle to aim for in production code, it's not as important in tests which tend to benefit more from a DAMP approach.
I'm not entirely averse to the idea of sharing definition code between them but at the least, I don't think we should introduce it in an ad-hoc manner like this, and we should instead discuss it within the team before deciding whether to proceed with it.
For now, please can you remove the use of the story definition and instead inline the relevant values in the test?
| display: flex; | ||
| gap: 20px; | ||
| justify-content: space-between; | ||
| padding: $grid-gap-phone 40px; |
There was a problem hiding this comment.
It looks like the horizontal padding will need an adjustment at mobile viewports. Note how the icon aligns with those above it:
We don't have explicit mobile designs for the Info Notice component, but it's evident the icons have less horizontal padding (see in Figma):
There was a problem hiding this comment.
Hmm... I realise the IB specifies this component should be placed in assets/js/modules/analytics-4/components/common/, but it's only going to be shown on the dashboard so I think it should in fact go in assets/js/modules/analytics-4/components/dashboard/.
Summary
Addresses issue:
Relevant technical choices
The figma designs did not include any responsive designs. So I verified the responsive behavior of existing, similar components and implemented a similar behavior.
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist