fix(dialog-full-screen): prevent horizontal overflow due to a child Form with a sticky footer #6742

Parsium · 2024-05-22T12:47:22Z

Proposed behaviour

Ensure when DialogFullscreen is rendered without padding, and contains a wrapped sticky form:

const MyDialog = () => {
  const MyStickyForm = () => (
    <Form stickyFooter>Content</Form>
  );
  
  return (
    <DialogFullscreen open disableContentPadding title="title">
      <MyStickyForm/>
    </DialogFullscreen>
  );
}

that:

No padding nor margins are added to DialogFullscreen's content wrapper - which caused content to overflow horizontally.
The content wrapper does not clip any overflowing content and continues to delegate handling this to the child Form.

Current behaviour

When DialogFullscreen is rendered without padding, and contains a wrapped sticky form, content overflows horizontally and the vertical scrollbar is not visible:

Checklist

QA

Tested in provided StackBlitz sandbox/Storybook
Add new Playwright test coverage if required
Carbon implementation matches Design System/designs
UI Tests GitHub check reviewed if required

Testing instructions

For comparing behaviour before and after these changes, please use the following demos when manually testing:

Demo of current behaviour:

Demo of bug

Demo of new behaviour:

Open Storybook locally with npm start
Open DialogFullscreen's test story "With Sticky Form"
Open controls, set disableContentPadding prop to true
Observe no horizontal scrollbar and that the vertical scrollbar is present again

Parsium · 2024-05-22T13:20:39Z

Note: This is a temporary fix for the reported bug.

I suggest revisiting the implementation of form modals in Carbon, as currently our modal components (namely Dialog, DialogFullScreen and Sidebar) are all coupled with forms to support sticky footers.

Removing this decoupling will make the component's code more maintainable by avoiding child iteration/CSS overrides. And allow consumers to compose the components more freely without worrying about side-effects, such as the original bug addressed in this PR. This would most likely be a breaking change.

I've raised a new ticket FE-6643 to address this.

Parsium · 2024-05-22T13:26:56Z

src/hooks/__internal__/useIsStickyFooterForm/useIsStickyFooterForm.ts

- React.Children.toArray(children).some(
- (child) =>
- React.isValidElement(child) &&
- (child.type as React.FunctionComponent)?.displayName ===
- Form.displayName &&
- child.props.stickyFooter
- ),
- [children]
- );


As this hook iterates over children, only Forms that are passed as direct React nodes will be checked. This is why CSS styling has been used instead to account when a form is wrapped

Parsium · 2024-05-22T13:32:00Z

src/components/sidebar/sidebar.component.tsx

+ const hasStickyFooter = useMemo(
+ () =>
+ React.Children.toArray(children).some(
+ (child) =>
+ React.isValidElement<FormProps>(child) && child.props.stickyFooter
+ ),
+ [children]
+ );


This will need updating as well at a future date, since any wrapped Forms won't be recognised

we could use DOM querying here

const hasStickyFooter = () => { const form = ref.current?.querySelector("[data-component='form']"); return form.classList.contains("sticky"); };

edleeks87

LGTM @Parsium, might be worth getting UX to review it as there's potentially some changes to the layout if they've rendered content above the form (not within). It's not great in master so I think these changes are fine imo (surely someone will have raised an issue by now if it wasn't) but still worth double checking

Parsium · 2024-05-28T10:04:32Z

LGTM @Parsium, might be worth getting UX to review it as there's potentially some changes to the layout if they've rendered content above the form (not within). It's not great in master so I think these changes are fine imo (surely someone will have raised an issue by now if it wasn't) but still worth double checking

@harpalsingh for context, currently adding content above or below the sticky footer form (not within it) will break the layout:

This is evident in master as well, and fixing it will require significant refactoring of how this pattern is implemented.

No consumer has raised an issue for this, as we assume most are currently adding additional content within the form rather than above/below it, but are you happy for us to address it later?

harpalsingh · 2024-05-30T08:03:33Z

@Parsium I've approved the review, but I do feel we should eventually look at the issue if the content is added above/below the form.

Parsium · 2024-06-04T10:45:31Z

@Parsium I've approved the review, but I do feel we should eventually look at the issue if the content is added above/below the form.

Thanks @harpalsingh, raised this as FE-6643

…orm with a sticky footer fix #6719

carbonci · 2024-06-05T09:02:19Z

🎉 This PR is included in version 136.0.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Parsium self-assigned this May 22, 2024

Parsium added Pending Review Pending QA No UX QA Required Squad Front End labels May 22, 2024

Parsium changed the title ~~Fe 6570/form scrollbar bug~~ @Parsium fix(dialog-full-screen): prevent horizontal overflow due to a child Form with a sticky footer May 22, 2024

Parsium changed the title ~~@Parsium fix(dialog-full-screen): prevent horizontal overflow due to a child Form with a sticky footer~~ fix(dialog-full-screen): prevent horizontal overflow due to a child Form with a sticky footer May 22, 2024

Parsium commented May 22, 2024

View reviewed changes

edleeks87 previously approved these changes May 23, 2024

View reviewed changes

Parsium added Pending UX QA and removed No UX QA Required labels May 23, 2024

mihai-albu-sage self-requested a review May 28, 2024 09:34

mihai-albu-sage previously approved these changes May 28, 2024

View reviewed changes

Parsium marked this pull request as ready for review May 28, 2024 09:51

Parsium requested review from a team as code owners May 28, 2024 09:51

Parsium added Passed Review and removed Pending Review labels May 28, 2024

harpalsingh added Passed UX QA and removed Pending UX QA labels May 30, 2024

Parsium dismissed stale reviews from mihai-albu-sage and edleeks87 via f7af444 June 4, 2024 09:16

Parsium force-pushed the FE-6570/form-scrollbar-bug branch 2 times, most recently from f7af444 to 7b012a3 Compare June 4, 2024 09:36

ZhuoyuJin approved these changes Jun 4, 2024

View reviewed changes

ZhuoyuJin added Passed QA and removed Pending QA labels Jun 4, 2024

Parsium added 3 commits June 4, 2024 16:05

fix(dialog-full-screen): prevent horizontal overflow due to a child F…

4fea1d5

…orm with a sticky footer fix #6719

refactor: remove useIsStickyFooterForm hook

f600de0

refactor(dialog-full-screen): amend test stories

efd0884

Parsium force-pushed the FE-6570/form-scrollbar-bug branch from 7b012a3 to efd0884 Compare June 4, 2024 15:05

mihai-albu-sage approved these changes Jun 5, 2024

View reviewed changes

Parsium merged commit 9a38bbe into master Jun 5, 2024
21 checks passed

Parsium deleted the FE-6570/form-scrollbar-bug branch June 5, 2024 08:58

carbonci added the released label Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dialog-full-screen): prevent horizontal overflow due to a child Form with a sticky footer #6742

fix(dialog-full-screen): prevent horizontal overflow due to a child Form with a sticky footer #6742

Parsium commented May 22, 2024 •

edited

Loading

Parsium commented May 22, 2024 •

edited

Loading

Parsium May 22, 2024 •

edited

Loading

Parsium May 22, 2024

edleeks87 May 22, 2024

edleeks87 left a comment

Parsium commented May 28, 2024

harpalsingh commented May 30, 2024

Parsium commented Jun 4, 2024

carbonci commented Jun 5, 2024

fix(dialog-full-screen): prevent horizontal overflow due to a child Form with a sticky footer #6742

fix(dialog-full-screen): prevent horizontal overflow due to a child Form with a sticky footer #6742

Conversation

Parsium commented May 22, 2024 • edited Loading

Proposed behaviour

Current behaviour

Checklist

QA

Testing instructions

Demo of current behaviour:

Demo of new behaviour:

Parsium commented May 22, 2024 • edited Loading

Parsium May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Parsium May 22, 2024

Choose a reason for hiding this comment

edleeks87 May 22, 2024

Choose a reason for hiding this comment

edleeks87 left a comment

Choose a reason for hiding this comment

Parsium commented May 28, 2024

harpalsingh commented May 30, 2024

Parsium commented Jun 4, 2024

carbonci commented Jun 5, 2024

Parsium commented May 22, 2024 •

edited

Loading

Parsium commented May 22, 2024 •

edited

Loading

Parsium May 22, 2024 •

edited

Loading