EES-5424 Add deleted and new location group tables to location mappings page #5171
Conversation
| @@ -73,7 +73,7 @@ export default function ApiDataSetMappableTable({ | |||
| return ( | |||
| <table | |||
| className="dfe-table--vertical-align-middle dfe-table--row-highlights" | |||
| id={`mappable-filter-options-${kebabCase(groupKey)}`} | |||
| id={`mappable-table-${camelCase(groupKey)}`} | |||
There was a problem hiding this comment.
Why the change from kebabCase to camelCase here? In the styleguide the preference is for kebab.
There was a problem hiding this comment.
I did this for consistency with location level keys (which are camelCased).
However, it's not really a big deal to go with kebab-casing so I'll switch back.
| import render from '@common-test/render'; | ||
| import { screen, within } from '@testing-library/react'; | ||
|
|
||
| describe('ApiDataSetMappingModal', () => { |
There was a problem hiding this comment.
This should be 'ApiDataSetLocationGroupOptionsModal'
| @@ -100,11 +115,27 @@ export default function ReleaseApiDataSetLocationsMappingPage() { | |||
|
|
|||
| const navItems: NavItem[] = useMemo(() => { | |||
| return [ | |||
| ...(Object.keys(deletedLocationGroups).length > 0 | |||
There was a problem hiding this comment.
I didn't add the filter column equivalents for this and new location groups to the nav on the filters mapping page, but it probably makes sense to add them in to make it consistent with the locations page.
| ...(Object.keys(deletedLocationGroups).length > 0 | ||
| ? [ | ||
| { | ||
| id: 'deleted-location-groups', |
There was a problem hiding this comment.
We've got a naming disparity between this and the filter columns, you've gone with 'deleted' whereas I went with 'mappable' on the basis that in the future you'll be able to map filter columns. Is the plan to be able to map location groups eventually? If so, it would be good to standardise the naming to avoid confusion.
There was a problem hiding this comment.
I've gone with deleted as I don't think there's any plans to make these mappable. There's a number of cases where mapping isn't really possible e.g. you can't turn a school into a region.
Also, changes to location groups aren't expected to be commonplace so it's generally more tolerable not to have mappings than it is for filter changes (where column renames can occur way more frequently).
| @@ -315,7 +315,7 @@ export default function ReleaseApiDataSetFiltersMappingPage() { | |||
| className="govuk-heading-l dfe-flex dfe-align-items--center" | |||
| id="mappable-filter-columns" | |||
| > | |||
| Filter columns not found in the new data set{' '} | |||
| Filter columns not found in new data set{' '} | |||
There was a problem hiding this comment.
This is entirely subjective, but I think this, and the other places you've changed it, read better with "the".
There was a problem hiding this comment.
I've gone with this wording as the extra word causes the heading to wrap for location groups. Given it's subjective and basically makes no real difference textually, I'd prefer we keep it this way as it keeps the styling consistent.
This PR adds additional tables for deleted and new location groups. These types of changes occur if a set of location columns are changed in some way e.g. local authority columns are removed or added.
We don't consider location groups to be mappable to one another, so only add or delete operations are possible.
The UI looks like the following:
Note the 'Location groups not found in new data set' and 'New location groups' sections have been added.
Clicking the 'View group options' button for one of these shows:
Other changes
codeelements. This was agreed with Marv although it hasn't been reflected in the prototype yet.codeelements as well.