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.

Sign up for GitHub

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

Jump to bottom

feat(FullscreenModal): add component #22

Merged
merged 5 commits into from
Dec 18, 2020
Merged

feat(FullscreenModal): add component #22

merged 5 commits into from
Dec 18, 2020

Conversation

ajkl2533
Copy link
Contributor

@ajkl2533 ajkl2533 commented Dec 14, 2020 •
edited
Loading

This PR is adding a component for FullscreenModal pattern.

Supported layouts

  • single-6 - single column layout (total width of 6 columns)
  • single-8 - single column layout (total width of 8 columns)
  • sidebar-4-6 - two columns layout (total width of 10 columns, 4 for sidebar and 6 for content)
  • sidebar-4-8 - two columns layout (total width of 12 columns, 4 for sidebar and 8 for content)

Usage

<FullscreenModal
  layout=LAYOUT_TYPE
  header=REACT.NODE // string or component
  content=REACT.NODE // string or component
  footer=REACT.NODE // string or component
  sidebar=REACT.NODE // string or component
/>

@ajkl2533 ajkl2533 force-pushed the ajkl2533@implement-fullscreen-modal branch 2 times, most recently from 9a59804 to 59de993 Compare December 15, 2020 14:00
@ajkl2533 ajkl2533 force-pushed the ajkl2533@implement-fullscreen-modal branch 2 times, most recently from ef0036d to e5623dd Compare December 16, 2020 10:42
abykovssc
abykovssc approved these changes Dec 16, 2020
View reviewed changes
Copy link
Contributor

@abykovssc abykovssc left a comment

Choose a reason for hiding this comment

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

LGTM, but one last thing to consider

src/components/FullscreenModal/hooks/useStickyObserver.ts Outdated Show resolved Hide resolved
@ajkl2533 ajkl2533 force-pushed the ajkl2533@implement-fullscreen-modal branch 6 times, most recently from 2395281 to 466876d Compare December 17, 2020 12:34
@ajkl2533 ajkl2533 force-pushed the ajkl2533@implement-fullscreen-modal branch from 466876d to cea34fb Compare December 17, 2020 13:44
@ajkl2533 ajkl2533 force-pushed the ajkl2533@implement-fullscreen-modal branch from cea34fb to 29f3d31 Compare December 18, 2020 08:57
vaclav-zeman
vaclav-zeman reviewed Dec 18, 2020
View reviewed changes
Copy link

@vaclav-zeman vaclav-zeman left a comment

Choose a reason for hiding this comment

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

Looks good, wish it could go without all the ref passing and complex observers but I don't have any better solution to offer

src/components/FullscreenModal/FullscreenModal.tsx Outdated Show resolved Hide resolved
src/components/FullscreenModal/FullscreenModal.tsx Show resolved Hide resolved
src/components/FullscreenModal/FullscreenModal.tsx Outdated Show resolved Hide resolved
src/components/FullscreenModal/Header/Header.tsx Show resolved Hide resolved
src/components/FullscreenModal/ScrollToTop/ScrollToTop.tsx Outdated Show resolved Hide resolved
src/components/FullscreenModal/ScrollToTop/ScrollToTop.tsx Outdated Show resolved Hide resolved
src/components/FullscreenModal/hooks/useStickyFooter.ts Outdated Show resolved Hide resolved
src/components/FullscreenModal/hooks/useStickyObserver.ts Show resolved Hide resolved
youssefssc
youssefssc approved these changes Dec 18, 2020
View reviewed changes
Copy link
Contributor

@youssefssc youssefssc left a comment

Choose a reason for hiding this comment

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

nice work LGTM

vaclav-zeman
vaclav-zeman approved these changes Dec 18, 2020
View reviewed changes
Copy link

@vaclav-zeman vaclav-zeman left a comment

Choose a reason for hiding this comment

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

LGTM

@ajkl2533 ajkl2533 force-pushed the ajkl2533@implement-fullscreen-modal branch from 367d8ab to a671f8c Compare December 18, 2020 11:01
@ajkl2533 ajkl2533 merged commit 6045a1f into alpha Dec 18, 2020
@ajkl2533 ajkl2533 deleted the ajkl2533@implement-fullscreen-modal branch December 18, 2020 11:30
@github-actions
Copy link

🎉 This PR is included in version 1.0.0-alpha.15 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjamine benjamine benjamine left review comments

@vaclav-zeman vaclav-zeman vaclav-zeman approved these changes

@abykovssc abykovssc abykovssc approved these changes

@youssefssc youssefssc youssefssc approved these changes

Assignees
No one assigned
Labels
released on @alpha
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ajkl2533 @benjamine @vaclav-zeman @abykovssc @youssefssc