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(web, web-twig, web-react): Introduce prop underline to Link component #1565

Merged
merged 5 commits into from
Aug 16, 2024

Conversation

pavelklibani
Copy link
Contributor

@pavelklibani pavelklibani commented Aug 5, 2024

Description

Additional context

  • Add new prop underline to Link component
  • Deprecate prop isUnderline in Link component
  • Update demo and stories for Link component
  • Visual tests

Issue reference

Podtržení odkazů na hover je někdy nežádoucí

@pavelklibani pavelklibani self-assigned this Aug 5, 2024
@github-actions github-actions bot added the feature New feature or request label Aug 5, 2024
Copy link

netlify bot commented Aug 5, 2024

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

Name Link
🔨 Latest commit 9b260f6
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-storybook/deploys/66bf024a42f5d2000863a51a

Copy link

netlify bot commented Aug 5, 2024

Deploy Preview for spirit-design-system ready!

Name Link
🔨 Latest commit 9b260f6
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system/deploys/66bf024aca80f8000849489d
😎 Deploy Preview https://deploy-preview-1565--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 Aug 5, 2024

Coverage Status

coverage: 78.535% (+0.3%) from 78.263%
when pulling 9b260f6 on feat/ds-943-link-underline
into 15a191d on main.

@pavelklibani pavelklibani marked this pull request as ready for review August 6, 2024 11:14
@pavelklibani pavelklibani force-pushed the feat/ds-943-link-underline branch from 102130c to f6b8bf2 Compare August 6, 2024 14:34
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.

Just a few nitpicks. As a whole it looks fine. Thanks 👍

packages/web-react/src/components/Link/Link.tsx Outdated Show resolved Hide resolved
packages/web-react/src/components/Link/README.md Outdated Show resolved Hide resolved
packages/web-twig/src/Resources/components/Link/README.md Outdated Show resolved Hide resolved
packages/web-react/src/types/link.ts Outdated Show resolved Hide resolved
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 update visual tests as well. The react should have a screenshot.

Also, I am a bit surprised the TS tests are working. You specify the prop in types as underlined, but use underline. Shouldn't it fail somewhere?

@pavelklibani pavelklibani force-pushed the feat/ds-943-link-underline branch from 8e2ffb6 to 8965d7f Compare August 8, 2024 14:09
@pavelklibani pavelklibani force-pushed the feat/ds-943-link-underline branch from 8965d7f to e79223a Compare August 8, 2024 14:21
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.

Good work!

We just need to document the deprecations more.

There should be mention in both of these files:

  • packages/web-react/DEPRECATIONS.md
  • packages/web-twig/DEPRECATIONS.md

If I remember correctly, there could be a migration guide as well. So we can then copy it to the migration instructions.

packages/web-twig/src/Resources/components/Link/README.md Outdated Show resolved Hide resolved
packages/web-react/src/types/link.ts Outdated Show resolved Hide resolved
packages/codemods/src/transforms/v3/web-react/README.md Outdated Show resolved Hide resolved
packages/web-react/src/types/link.ts Show resolved Hide resolved
@pavelklibani
Copy link
Contributor Author

Good work!

We just need to document the deprecations more.

There should be mention in both of these files:

  • packages/web-react/DEPRECATIONS.md
  • packages/web-twig/DEPRECATIONS.md

If I remember correctly, there could be a migration guide as well. So we can then copy it to the migration instructions.

Deprecation files updated, thank you for remebering!

- Add new prop `underlined` to Link component
- Deprecate prop `isUnderlined` in Link component
- Update demo and stories for Link component

Solves #DS-943
- Add new prop `underlined` to Link component
- Deprecate prop `isUnderlined` in Link component
- Update demo for Link component

Solves #DS-943
- replacing the `isUnderlined` prop with a new `underlined` prop
and set it to "always" if true
@pavelklibani pavelklibani force-pushed the feat/ds-943-link-underline branch from b7da21c to 9b260f6 Compare August 16, 2024 07:39
@pavelklibani pavelklibani merged commit 77a5ec5 into main Aug 16, 2024
27 checks passed
@pavelklibani pavelklibani deleted the feat/ds-943-link-underline branch August 16, 2024 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants