EES-5419 import new data set version #5152
Conversation
amyb-hiveit
requested review from
benoutram,
ntsim,
duncan-at-hiveit and
jack-hive
| (dataSet?.draftVersion?.status === 'Draft' || | ||
| dataSet?.draftVersion?.status === 'Mapping'); | ||
|
|
||
| const getImportBanner = () => { |
There was a problem hiding this comment.
Would split out a separate component to wrap up all these different banner states
There was a problem hiding this comment.
I've moved this to its own component now.
| return ( | ||
| <NotificationBanner | ||
| fullWidthContent | ||
| heading="This new API data set version has been mapped and is ready to be imported" |
There was a problem hiding this comment.
Would suggest changing this to 'Draft API data set version is ready to be finalised'?
There was a problem hiding this comment.
Changed to 'finalise' consistently throughout now.
| const [importingStatus, setImportingStatus] = useState< | ||
| 'importing' | 'imported' | undefined | ||
| >(undefined); |
There was a problem hiding this comment.
We already have a bunch of things that involve 'importing', so it's maybe a bit confusing to reference it like this. Additionally, the backend references it as 'completing' a data set version.
Would suggest changing this to at least completingStatus, but as I mention a bit later, 'finalise' (i.e finalisingStatus) might also do.
| return ( | ||
| <NotificationBanner | ||
| fullWidthContent | ||
| heading="Import in progress..." |
There was a problem hiding this comment.
Would be more explicit here about it relating to the draft version.
If we go with my other comment's suggestion to reference 'import' as 'finalise', then we could reword this to 'Finalising draft API data set version'. Otherwise, 'Importing draft API data set version' would do.
| heading="Import in progress..." | ||
| title="Importing" | ||
| > | ||
| <p>This process can take a few minutes</p> |
There was a problem hiding this comment.
Minor, but would add a full stop.
| notes and also preview this API data set version. | ||
| </p> | ||
| <p className="govuk-!-margin-bottom-6"> | ||
| <strong>Please note</strong> you will not be able to make further |
There was a problem hiding this comment.
No need to say 'Please note' - see GOV.UK content writing guidance on Tone.
| remove this API data set version up until the release publication. | ||
| </p> | ||
| <p> | ||
| <strong>Please note</strong> these changes will not be made in the |
There was a problem hiding this comment.
No need to say 'Please note' - see GOV.UK content writing guidance on Tone.
| <ErrorSummary | ||
| errors={[ | ||
| { | ||
| id: 'draft-version-summary', | ||
| message: 'Data set version import failed', | ||
| }, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
Do we need to use error summary for this? We're not on a form, so it's a bit weird and inconsistent given we've already been using the notification banner for the other states.
Could just add an error variant of NotificationBanner?
There was a problem hiding this comment.
Changed to use NotificationBanner now.
| @@ -396,10 +399,209 @@ describe('ReleaseApiDataSetDetailsPage', () => { | |||
| ).not.toBeInTheDocument(); | |||
| }); | |||
|
|
|||
| describe('importing', () => { | |||
There was a problem hiding this comment.
If we go with the 'finalise' terminology, would make any relevant text changes to this test suite here and below.
| <NotificationBanner | ||
| fullWidthContent | ||
| heading="This new API data set version has now been mapped and imported ready for final publication" | ||
| role={importingStatus === 'imported' ? 'alert' : undefined} |
There was a problem hiding this comment.
Not entirely sure, but changing the alert role like this might not trigger the banner to be read out. Think the alert only works if the browser is aware of the element (with role="alert") before its text is updated. From the Mozilla docs:
As with all other live regions, alerts will only be announced when the content of the element with role="alert" is updated. Make sure that the element with the role is present in the page's markup first - this will "prime" the browser and screen reader to keep watching the element for changes. After this, any changes to the content will be announced. Do not try to dynamically add/generate an element with role="alert" that is already populated with the alert message you want announced - this generally does not lead to an announcement, as it is not a content change.
I'm not sure if the notification banner element ends up being unmounted from the DOM at any point, so the browser may not detect the element's contents change changing.
Probably worth testing with a screen reader.
There was a problem hiding this comment.
It does work here because the role isn't changed dynamically - it only has the role alert when the import process has just finished (finalisingStatus === 'finalised'), when you visit the page again there isn't a finalisingStatus so the role isn't applied.
There was a problem hiding this comment.
Okay, will trust that you're right about this!
Adds the ability to import a new API data set version once the mappings have been completed. The notification banner goes through the following stages:
Before import:
While importing:
On successful import - changes to green and has `role="alert":
Coming back to the page after import - back to blue and no alert as it's not the result of an action you've just taken:
After discussion with Cam I've removed the button to re-import the version and updated the wording.