EES-4510 release type modal on release page #4306
Conversation
amyb-hiveit
requested review from
benoutram,
ntsim,
duncan-at-hiveit and
a user
| // eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex | ||
| tabIndex={0} |
There was a problem hiding this comment.
Rather than need the tabIndex, could we sort the scrolling out so that the entire window doesn't scroll when the modal is open?
There are various modal (or dialog) implementations that handle this in slightly different ways:
The main approach seems to be to overflow: hidden the body when the modal opens.
I'd suggest we go a little further and replace the underlying implementation with Radix's as it seems to be getting more support than react-modal. For example, this issue seems to suggest that it won't work with React 18, making itanother blocker for upgrading.
If we swap out the implementation, we shouldn't need to do any messing about with the scroll locking ourselves and we can also remove stuff like having to set process.env.APP_ROOT_ID.
There was a problem hiding this comment.
The tabIndex here is for the scrollable area inside the modal, not the modal itself (the screenshot is of how it works currently). If we made the whole modal scrollable (is that what you're suggesting?) I don't think it would work as well as the close button wouldn't be visible until you scroll to the bottom.
There was a problem hiding this comment.
Fair enough, we can keep the tabIndex if we want to always show the Close button. However, it's still be a good idea to lock the scrolling on the main window viewport as it's a bit weird to have two scrollbars.
I wonder if it'd be possible if we could scroll the modal content when scrolling outside the modal i.e. on the overlay. Not super necessary though.
There was a problem hiding this comment.
Scrolling - I've actually changed my mind now after testing them out on a few more screen sizes and have changed it to scroll the whole modal instead of just the content section.
I've started switching over to Radix but it's going to be a lot of changes so I'll do that in a follow up PR instead of this one (https://dfedigital.atlassian.net/browse/EES-4567). That'll sort the scroll locking issue as well.
| @@ -11,6 +12,8 @@ export interface ModalProps { | |||
| fullScreen?: boolean; | |||
| hideTitle?: boolean; | |||
| open?: boolean; | |||
| scrollableContent?: boolean; | |||
| showCloseButton?: boolean; | |||
There was a problem hiding this comment.
- Would suggest simplifying this to
showClose. This would be more consistent withModalConfirm - Could add an optional
cancelTextprop (defaulting to 'Close')
| @@ -1,8 +1,12 @@ | |||
| import React from 'react'; | |||
|
|
|||
| const AdHocOfficialStatisticsSection = () => ( | |||
| const AdHocStatisticsSection = ({ | |||
There was a problem hiding this comment.
- Could
export default functionthis. Same for the other section components in this PR as well. - All the section components are in
modules/find-statistics. A more appropriate place would bemodules/release.
It actually seems like all the find-statistics module files could be moved over to the release module. Would suggest we do a quick follow up PR with this change to clean this up finally.
There was a problem hiding this comment.
I've moved these and will do a tidy up PR for the rest later.
| onToggleReleaseTypeModal?: () => void; | ||
| } | ||
|
|
||
| export default function BasicReleaseSummary({ |
There was a problem hiding this comment.
Perhaps we could name this a bit better e.g. ReleaseSummaryList or ReleaseSummarySection.
It seems a bit weird to call it BasicReleaseSummary as there isn't a more complicated version of this component. It could also be a little confusing if we introduce a more trimmed ReleaseSummary type in the future too.
| altText: string; | ||
| } | ||
|
|
||
| const nationalStatisticsLogo: ReleaseTypeIcon = { |
There was a problem hiding this comment.
This can be just inlined into releaseTypesToIcons
| altText: 'UK statistics authority quality mark', | ||
| }; | ||
|
|
||
| const releaseTypesToIcons: Dictionary<ReleaseTypeIcon> = { |
There was a problem hiding this comment.
A better type for this would be Partial<Record<ReleaseType, ReleaseTypeIcon>>
| renderReleaseNotes: ReactNode; | ||
| renderStatusTags: ReactNode; | ||
| renderSubscribeLink: ReactNode; | ||
| onToggleReleaseTypeModal?: () => void; |
There was a problem hiding this comment.
onShowReleaseTypeModal might be better. onToggle implies the event should fire when the modal closes too.
| </SummaryList> | ||
|
|
||
| <Modal | ||
| className="govuk-!-width-one-half" |
There was a problem hiding this comment.
Not sure this responds too great on smaller desktops / tablets:
It doesn't seem like we really need this at all personally.
Without the width:
With the width:
| export const releaseTypeComponents = { | ||
| NationalStatistics: NationalStatisticsSection, | ||
| OfficialStatistics: OfficialStatisticsSection, | ||
| ExperimentalStatistics: ExperimentalStatisticsSection, | ||
| AdHocStatistics: AdHocStatisticsSection, | ||
| ManagementInformation: ManagementInformationSection, | ||
| }; |
There was a problem hiding this comment.
Doesn't seem appropriate to put this in here. This is the services directory, so we shouldn't need to put React components in here.
These are also quite specific to the section components for the release pages, which doesn't make a lot of sense for a file that will be used more generally.
Would suggest we just create a dedicated ReleaseTypeSection component that takes a type prop and wraps these other sections. We might even want to just move all the other section components into the same file for simplicity (up to you).
|
|
||
| .content { | ||
| margin-bottom: govuk-spacing(5); | ||
| max-height: 50vh; |
There was a problem hiding this comment.
Would it be possible to bump this up some more? Something like 70vh could be better.
There was a problem hiding this comment.
This has been removed now.
amyb-hiveit
force-pushed
the
EES-4510
branch
2 times, most recently
from
8b64936
to
873e534
Compare
|
|
||
| export interface ModalProps { | ||
| cancelText?: string; |
There was a problem hiding this comment.
Sorry my bad, but I actually meant closeText! Could move this next to the other close props too
| import { ReleaseType } from '@common/services/types/releaseType'; | ||
| import React from 'react'; | ||
|
|
||
| export const releaseTypeComponents = { |
There was a problem hiding this comment.
These don't need to be exported anymore
amyb-hiveit
force-pushed
the
EES-4510
branch
from
873e534
to
1c53e93
Compare
Moves the release type from a tag at the top to the summary list. Clicking it will open a modal containing information about the release type.
I've done a bit of tidying to reduce inconsistencies between the admin and public versions of the release pages:
BasicReleaseSummarycomponent for use in both and removed the admin only oneReleaseHelpAndSupportSectioncomponents into oneAlso, I've made a change to the
Modalcomponent to make it simpler to have the content be scrollable and have a close button. This is used on the Glossary and Find Stats - Filters modals as well as the new one for release type.