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: Add Message and Link for ToastBar #DS-1213 #1426

Merged
merged 3 commits into from
May 28, 2024
Merged

Feat: Add Message and Link for ToastBar #DS-1213 #1426

merged 3 commits into from
May 28, 2024

Conversation

curdaj
Copy link
Contributor

@curdaj curdaj commented May 20, 2024

Description

Additional context

Issue reference

@netlify Netlify
Copy link

netlify bot commented May 20, 2024 •
edited

✅ Deploy Preview for spirit-design-system ready!

Name Link
🔨 Latest commit 67cdd35
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system/deploys/6655be124df9600008c93da0
😎 Deploy Preview https://deploy-preview-1426--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.

@coveralls
Copy link

coveralls commented May 20, 2024 •
edited

Coverage Status

coverage: 78.687% (-0.5%) from 79.154%
when pulling 67cdd35 on feat/ds-1213-link-and-message-in-toastbar
into d0625a8 on main.

@curdaj curdaj force-pushed the feat/ds-1213-link-and-message-in-toastbar branch from 03a0564 to fee3b3c Compare May 20, 2024 09:20
@curdaj curdaj changed the title Feat: Add Message and Link for ToastBar #DS-1213 🚧 Feat: Add Message and Link for ToastBar #DS-1213 May 20, 2024
@curdaj curdaj changed the title 🚧 Feat: Add Message and Link for ToastBar #DS-1213 Feat: Add Message and Link for ToastBar #DS-1213 May 21, 2024
crishpeen
crishpeen requested changes May 21, 2024
View reviewed changes
packages/web-twig/src/Resources/components/Toast/ToastBarMessage.twig Show resolved Hide resolved
...ages/web-twig/src/Resources/components/Toast/__tests__/__snapshots__/toastBar.twig.snap.html Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Toast/ToastBarLink.twig Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Toast/ToastBar.twig Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Toast/ToastBar.twig Show resolved Hide resolved
packages/web-react/src/components/Toast/__tests__/ToastBarLink.test.tsx Show resolved Hide resolved
packages/web-react/src/components/Toast/ToastBarMessage.tsx Outdated Show resolved Hide resolved
packages/web-react/src/components/Toast/README.md Outdated Show resolved Hide resolved
packages/web-react/src/components/Toast/README.md Show resolved Hide resolved
packages/web-react/src/components/Toast/README.md Outdated Show resolved Hide resolved
@curdaj curdaj force-pushed the feat/ds-1213-link-and-message-in-toastbar branch from 3c7b669 to 998af72 Compare May 21, 2024 14:16
@curdaj curdaj force-pushed the feat/ds-1213-link-and-message-in-toastbar branch 2 times, most recently from 74a20d2 to 1d75276 Compare May 22, 2024 10:08
@curdaj curdaj requested review from literat and crishpeen May 22, 2024 10:10
@curdaj curdaj marked this pull request as ready for review May 22, 2024 10:10
@curdaj curdaj requested review from adamkudrna, pavelklibani and a team as code owners May 22, 2024 10:10
@curdaj curdaj force-pushed the feat/ds-1213-link-and-message-in-toastbar branch from 6957412 to e8aa8bd Compare May 22, 2024 10:14
@curdaj curdaj requested a review from literat May 22, 2024 11:38
crishpeen
crishpeen requested changes May 23, 2024
View reviewed changes
Copy link
Member

@crishpeen crishpeen left a comment

Choose a reason for hiding this comment

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

I checked web and twig so far :)

packages/web/src/scss/components/Toast/README.md Show resolved Hide resolved
packages/web/src/scss/components/Toast/README.md Outdated Show resolved Hide resolved
packages/web/src/scss/components/Toast/README.md Outdated Show resolved Hide resolved
packages/web/src/scss/components/Toast/README.md Outdated Show resolved Hide resolved
packages/web/src/scss/components/Toast/README.md Outdated Show resolved Hide resolved
packages/web/src/scss/components/Toast/_ToastBar.scss Show resolved Hide resolved
packages/web/src/scss/components/Toast/index.html Outdated Show resolved Hide resolved
packages/web/src/scss/components/Toast/index.html Show resolved Hide resolved
packages/web/src/scss/components/Toast/index.html Show resolved Hide resolved
packages/web-twig/src/Resources/components/Toast/ToastBarLink.twig Show resolved Hide resolved
crishpeen
crishpeen reviewed May 23, 2024
View reviewed changes
Copy link
Member

@crishpeen crishpeen left a comment

Choose a reason for hiding this comment

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

Please add it to the demos, so we can see it works :) and then we will probably need to modify the screenshot for e2e tests

packages/web-react/src/components/Toast/README.md Show resolved Hide resolved
packages/web-react/src/components/Toast/__tests__/ToastBarLink.test.tsx Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Toast/ToastBarLink.twig Outdated Show resolved Hide resolved
@curdaj curdaj force-pushed the feat/ds-1213-link-and-message-in-toastbar branch from a8f152e to 8983bbd Compare May 24, 2024 08:18
@curdaj curdaj changed the base branch from integration/release-v2 to main May 24, 2024 08:19
@curdaj curdaj changed the base branch from main to integration/release-v2 May 24, 2024 08:20
@curdaj curdaj changed the base branch from integration/release-v2 to main May 24, 2024 08:20
@crishpeen crishpeen force-pushed the feat/ds-1213-link-and-message-in-toastbar branch from 8983bbd to 22d080f Compare May 24, 2024 08:25
@netlify Netlify
Copy link

netlify bot commented May 24, 2024 •
edited

✅ Deploy Preview for spirit-design-system-storybook canceled.

Name Link
🔨 Latest commit 67cdd35
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-storybook/deploys/6655be128206b0000867339f

@curdaj curdaj force-pushed the feat/ds-1213-link-and-message-in-toastbar branch from 22d080f to eb1971d Compare May 24, 2024 08:37
@curdaj curdaj requested a review from crishpeen May 27, 2024 11:50
@literat
Copy link
Collaborator

literat commented May 27, 2024

@curdaj Please, update the web-twig components snapshots. It will fix failing tests.
@crishpeen Please, check that all your suggestions are resolved. In future issues, we must do better work splitting or defining must-have parts. This PR rose bigger than expected.

I do not see here any crucial flaws and it is looking good from my perspective. :-)

pavelklibani
pavelklibani approved these changes May 27, 2024
View reviewed changes
Copy link
Contributor

@pavelklibani pavelklibani left a comment

Choose a reason for hiding this comment

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

LGTM 👍

crishpeen
crishpeen reviewed May 28, 2024
View reviewed changes
packages/web-react/src/components/Toast/ToastBarLink.tsx Outdated Show resolved Hide resolved
packages/web-react/src/components/Toast/ToastBarLink.tsx Outdated Show resolved Hide resolved
packages/web-react/src/components/Toast/__tests__/UncontrolledToast.test.tsx Show resolved Hide resolved
packages/web-react/src/components/Toast/demo/ToastDynamicToastQueue.tsx Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Toast/ToastBarLink.twig Show resolved Hide resolved
packages/web-react/src/components/Toast/README.md Show resolved Hide resolved
...ages/web-twig/src/Resources/components/Toast/__tests__/__snapshots__/toastBar.twig.snap.html Outdated Show resolved Hide resolved
...ages/web-twig/src/Resources/components/Toast/__tests__/__snapshots__/toastBar.twig.snap.html Show resolved Hide resolved
packages/web/src/js/Toast.ts Outdated Show resolved Hide resolved
apps/web-twig-demo/assets/scripts/toast-dynamic.ts Outdated Show resolved Hide resolved
crishpeen
crishpeen approved these changes May 28, 2024
View reviewed changes
Copy link
Member

@crishpeen crishpeen left a comment

Choose a reason for hiding this comment

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

Please add Message storybook story too, please.

Otherwise LGTM! Great work!

@curdaj curdaj added the run-visual-tests Runs visual regression testing on this PR label May 28, 2024
@curdaj curdaj force-pushed the feat/ds-1213-link-and-message-in-toastbar branch from b110949 to 67cdd35 Compare May 28, 2024 11:20
@curdaj curdaj merged commit 0a497a6 into main May 28, 2024
31 checks passed
@curdaj curdaj deleted the feat/ds-1213-link-and-message-in-toastbar branch May 28, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@literat literat literat left review comments

@crishpeen crishpeen crishpeen approved these changes

@pavelklibani pavelklibani pavelklibani approved these changes

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

Assignees
No one assigned
Labels
run-visual-tests Runs visual regression testing on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@curdaj @coveralls @literat @crishpeen @pavelklibani