Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(modal): Support scrolling entire modal #905

Conversation

miguelc1221
Copy link

Summary

This PR adds support for scrolling long content on modal. #790

Checklist

  • branch has been rebased on the latest master commit
  • yarn test passes
  • module has been added to canvas-kit-react and/or canvas-kit-css universal modules, if
    applicable
  • design approved final implementation
  • a11y approved final implementation
  • code adheres to the API & Pattern guidelines

Additional References

scroll moda

minHeight: 'calc(100% - (1.75rem * 2))',

// IE11 fix for setting min-height in a flex container
'&::before': {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IE11 fix for min-height philipwalton/flexbugs#231

@@ -184,6 +191,18 @@ const useInitialFocus = (
}, [modalRef, firstFocusRef]);
};

const useRemoveBodyScroll = () => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a hook to remove scroll on body when modal is open

@cypress
Copy link

cypress bot commented Nov 16, 2020



Test summary

560 0 1 0


Run details

Project canvas-kit
Status Passed
Commit 7f480d6 ℹ️
Started Nov 16, 2020 4:37 AM
Ended Nov 16, 2020 4:42 AM
Duration 05:24 💡
OS Linux Ubuntu Linux - 18.04
Browser Electron 80

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

alignItems: 'center',
justifyContent: 'center',
margin: '1.75rem auto',
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used 1.75rem as the margin for the top and bottom value (same as bootstrap), as i am unsure how much space to put when the model is higher than viewport

@lychyi lychyi added 5.x question Further information is requested labels Nov 16, 2020
@NicholasBoll NicholasBoll self-assigned this Nov 16, 2020
@NicholasBoll
Copy link
Member

Cool. Thanks for this. I'll look into these changes for the non-full modal scroll as well. We have plans to update the Modal component to support more use-cases through composability without adding more config options and we'll see how these changes fit into that goal.

@jpante jpante added this to Open in Backlog Dec 7, 2020
@jpante jpante moved this from Open to Up Next in Backlog Jan 4, 2021
@jpante jpante added this to the 5.0.0 milestone Jan 19, 2021
@jpante jpante added the p:2 label Mar 1, 2021
@project-bot project-bot bot moved this from Up Next to Medium in Backlog Mar 1, 2021
@willklein
Copy link
Contributor

Sorry we weren't able to merge this. If anyone finds it should be picked up we can re-open it or you can create a new PR incorporating the relevant changes.

@willklein willklein closed this Oct 12, 2021
@willklein
Copy link
Contributor

It also looks like #1259 will address this based on recent changes to our modal component.

@NicholasBoll NicholasBoll removed this from the 5.0.0 milestone Jul 22, 2022
@jaclynjessup jaclynjessup removed the p:2 label Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.x question Further information is requested
Projects
Archived in project
Backlog
  
Medium
Development

Successfully merging this pull request may close these issues.

None yet

6 participants