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(web, web-twig, web-react): Hide close button in Modal #1535

Merged
merged 4 commits into from
Jul 9, 2024
Merged

Feat(web, web-twig, web-react): Hide close button in Modal #1535

merged 4 commits into from
Jul 9, 2024

Conversation

pavelklibani
Copy link
Contributor

@pavelklibani pavelklibani commented Jul 2, 2024 •
edited
Loading

Description

  • Added a new prop to hide the close button in the ModalHeader.
  • Added a new prop to disable the escape key from closing the modal.

Additional context

TODO:

  • Update visual tests
  • Update storybook

Issue reference

Modal: komponenta nemusí mít zavírací tlačítko

@pavelklibani pavelklibani self-assigned this Jul 2, 2024
@github-actions github-actions bot added the feature New feature or request label Jul 2, 2024
@netlify Netlify
Copy link

netlify bot commented Jul 2, 2024 •
edited
Loading

✅ Deploy Preview for spirit-design-system ready!

Name Link
🔨 Latest commit 3ba9588
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system/deploys/668d0b0c759ef700088164cc
😎 Deploy Preview https://deploy-preview-1535--spirit-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse		 1 paths audited
Performance: 96 (no change from production)
Accessibility: 93 (no change from production)
Best Practices: 100 (no change from production)
SEO: 82 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify Netlify
Copy link

netlify bot commented Jul 2, 2024 •
edited
Loading

✅ Deploy Preview for spirit-design-system-storybook ready!

Name Link
🔨 Latest commit 3ba9588
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-storybook/deploys/668d0b0c46fbf500083f5b0f
😎 Deploy Preview https://deploy-preview-1535--spirit-design-system-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@coveralls
Copy link

Coverage Status

coverage: 78.921% (+0.8%) from 78.141%
when pulling cc82fec on feat/ds-1063-modal-hide-close-button
into 86ba5ff on main.

@pavelklibani pavelklibani force-pushed the feat/ds-1063-modal-hide-close-button branch from cc82fec to 2e55023 Compare July 2, 2024 13:48
@coveralls
Copy link

Coverage Status

coverage: 78.921% (+0.8%) from 78.141%
when pulling 2e55023 on feat/ds-1063-modal-hide-close-button
into 86ba5ff on main.

@coveralls
Copy link

Coverage Status

coverage: 77.951% (-0.2%) from 78.141%
when pulling 2e55023 on feat/ds-1063-modal-hide-close-button
into 86ba5ff on main.

@pavelklibani pavelklibani force-pushed the feat/ds-1063-modal-hide-close-button branch from 2e55023 to 1f30daa Compare July 2, 2024 14:25
@coveralls
Copy link

Coverage Status

coverage: 78.921% (+0.8%) from 78.141%
when pulling 1f30daa on feat/ds-1063-modal-hide-close-button
into 86ba5ff on main.

@coveralls
Copy link

Coverage Status

coverage: 77.785% (-0.4%) from 78.141%
when pulling 1f30daa on feat/ds-1063-modal-hide-close-button
into 86ba5ff on main.

@pavelklibani pavelklibani force-pushed the feat/ds-1063-modal-hide-close-button branch from 1f30daa to 9bff159 Compare July 3, 2024 10:11
@pavelklibani pavelklibani marked this pull request as ready for review July 3, 2024 10:12
@coveralls
Copy link

Coverage Status

coverage: 78.921% (+0.8%) from 78.141%
when pulling 9bff159 on feat/ds-1063-modal-hide-close-button
into 86ba5ff on main.

@pavelklibani pavelklibani changed the title [🚧 WIP] Feat(web, web-twig, web-react): Hide close button in Modal Feat(web, web-twig, web-react): Hide close button in Modal Jul 3, 2024
@coveralls
Copy link

Coverage Status

coverage: 77.785% (-0.4%) from 78.141%
when pulling 9bff159 on feat/ds-1063-modal-hide-close-button
into 86ba5ff on main.

@coveralls
Copy link

Coverage Status

coverage: 78.921% (+0.8%) from 78.141%
when pulling 5927a9f on feat/ds-1063-modal-hide-close-button
into 86ba5ff on main.

@coveralls
Copy link

Coverage Status

coverage: 77.785% (-0.4%) from 78.141%
when pulling 5927a9f on feat/ds-1063-modal-hide-close-button
into 86ba5ff on main.

@pavelklibani pavelklibani force-pushed the feat/ds-1063-modal-hide-close-button branch from 5927a9f to f7f287d Compare July 3, 2024 10:37
@coveralls
Copy link

Coverage Status

coverage: 78.921% (+0.8%) from 78.141%
when pulling f7f287d on feat/ds-1063-modal-hide-close-button
into 86ba5ff on main.

@coveralls
Copy link

Coverage Status

coverage: 77.785% (-0.4%) from 78.141%
when pulling f7f287d on feat/ds-1063-modal-hide-close-button
into 86ba5ff on main.

@literat
Copy link
Collaborator

literat commented Jul 3, 2024

Great Job 👍 It is working now. I just wonder if there is really some issue with the Chrome Browser and we should report it or if this is a problem with our logic. Since this was working in Safari and Firefox with just event.preventDefault maybe it should be reported by us :-)

@coveralls
Copy link

Coverage Status

coverage: 78.921% (+0.8%) from 78.141%
when pulling 4c4d8ef on feat/ds-1063-modal-hide-close-button
into 86ba5ff on main.

@coveralls
Copy link

Coverage Status

coverage: 77.797% (-0.3%) from 78.141%
when pulling 4c4d8ef on feat/ds-1063-modal-hide-close-button
into 86ba5ff on main.

literat
literat approved these changes Jul 3, 2024
View reviewed changes
Copy link
Collaborator

@literat literat left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Please, solve with the others the prop names. 🙏

@pavelklibani pavelklibani force-pushed the feat/ds-1063-modal-hide-close-button branch 3 times, most recently from c000352 to 1f44993 Compare July 9, 2024 08:13
@coveralls
Copy link

coveralls commented Jul 9, 2024 •
edited
Loading

Coverage Status

coverage: 77.797% (-1.3%) from 79.061%
when pulling 3ba9588 on feat/ds-1063-modal-hide-close-button
into ebd2b3f on main.

crishpeen
crishpeen approved these changes Jul 9, 2024
View reviewed changes
packages/web-react/src/components/Modal/README.md Outdated Show resolved Hide resolved
packages/web-react/src/components/Modal/README.md Outdated Show resolved Hide resolved
packages/web-react/src/components/Modal/__tests__/ModalHeader.test.tsx Outdated Show resolved Hide resolved
packages/web-react/src/components/Modal/__tests__/ModalHeader.test.tsx Outdated Show resolved Hide resolved
packages/web-react/src/components/Modal/demo/ModalHiddenCloseButton.tsx Outdated Show resolved Hide resolved
packages/web-react/src/hooks/useCancelEvent.ts Outdated Show resolved Hide resolved
packages/web-react/src/types/modal.ts Show resolved Hide resolved
packages/web-twig/src/Resources/components/Modal/README.md Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Modal/README.md Outdated Show resolved Hide resolved
packages/web/src/js/Modal.ts Show resolved Hide resolved
@pavelklibani pavelklibani force-pushed the feat/ds-1063-modal-hide-close-button branch from 4b2c9c8 to 6af6279 Compare July 9, 2024 09:45
dlouhak
dlouhak approved these changes Jul 9, 2024
View reviewed changes
packages/web-react/src/hooks/useCancelEvent.ts Outdated Show resolved Hide resolved
packages/web/src/js/Modal.ts Outdated Show resolved Hide resolved
@pavelklibani
Feat(web): ModalHeader hide close button prop DS-1063
670e1e5 
- New prop for ModalHeader to hide the close button
- New prop for disable escape key to close the modal
- Added new demo with those new props
@pavelklibani
Feat(web-twig): ModalHeader hide close button prop DS-1063
6dc409c 
- New prop for ModalHeader to hide the close button
- New prop for disable escape key to close the modal
- Added new demo with those new props
@pavelklibani
Feat(web-react): ModalHeader hide close button prop DS-1063
e275b83 
- New prop for ModalHeader to hide the close button
- New prop for disable escape key to close the modal
- Added new demo with those new props
@pavelklibani
Test(e2e): Update snapshot for modal component
3ba9588
@pavelklibani pavelklibani force-pushed the feat/ds-1063-modal-hide-close-button branch from 6af6279 to 3ba9588 Compare July 9, 2024 10:03
@pavelklibani pavelklibani merged commit 46ba736 into main Jul 9, 2024
29 checks passed
@pavelklibani pavelklibani deleted the feat/ds-1063-modal-hide-close-button branch July 9, 2024 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crishpeen crishpeen crishpeen approved these changes

@dlouhak dlouhak dlouhak approved these changes

@literat literat literat approved these changes

@curdaj curdaj curdaj approved these changes

@adamkudrna adamkudrna Awaiting requested review from adamkudrna adamkudrna is a code owner

Assignees

@pavelklibani pavelklibani

Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pavelklibani @coveralls @literat @crishpeen @dlouhak @curdaj