Implement selection panel add group notice #8881
Conversation
|
Build files for 227104d have been deleted. |
|
Size Change: +9.17 kB (+0.6%) Total Size: 1.55 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Hi @nfmohit, nice work here so far. However - it doesn't fully meet the AC. I realise the IB itself doesn't fully meet the AC as originally specced, so no worries, these things happen!
Anyhow, the notice should only be displayed if there is just one audience selected when the panel is initially opened, rather than any time the audience selection count in the panel is reduced to one.
The idea is that we want to draw the user's attention to this if they have a saved selection of just one audience, but not nag them too much while they are simply interacting with the selection.
One a related note - I realise this aspect wasn't made clear at all but once a user has selected another audience, the notice should disappear and should not show up again while the panel is still open even if the user reduces the selection to a single audience. I've amended the AC to make this point clear, sorry this was a last minute addition.
|
Thank you for the clarification, @techanvil! I've updated the PR accordingly. Please let me know if this looks good now. |
There was a problem hiding this comment.
Thanks @nfmohit!
I've left a comment on one of the tests, please take look.
Beyond that - I must admit that when testing it, the behaviour does feel slightly odd. Notably the case where the user opens the selection panel with a single saved audience, and then unchecks the audience.
At this point, the notice disappears, but the user hasn't actually given an implicit indication of having taken action in accordance with the notice - they've removed their audience rather than adding one as the notice suggests.
Anyhow, let's not worry about that here - I will raise a separate issue to refine the behaviour to address this.
| ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| it( 'should not display notice when there is a saved selection of less than or more than one group', async () => { |
There was a problem hiding this comment.
This describes the case where there is less than one group in the saved selection, but doesn't test it.
Please either add test coverage for the zero-selection case (maybe splitting the test into two or using it.each()), or amend the description.
There was a problem hiding this comment.
I've updated the test coverage using it.each() as recommended, thanks Tom!
Just noting I've added #8909 to address the above. |
|
I agree, thank you for covering this behaviour. I've addressed the CR feedback, thanks @techanvil ! |
Summary
Addresses issue:
Relevant technical choices
This PR implements an add group notice to the Audience Selection Panel when there is only one group selected.
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