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(styles): Added post-linkarea component (3830) #4030

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

veyaromain
Copy link
Contributor

@veyaromain veyaromain commented Nov 20, 2024

No description provided.

@veyaromain veyaromain requested review from a team as code owners November 20, 2024 07:30
@veyaromain veyaromain linked an issue Nov 20, 2024 that may be closed by this pull request
Copy link

changeset-bot bot commented Nov 20, 2024

🦋 Changeset detected

Latest commit: 97455a2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@swisspost/design-system-documentation Minor
@swisspost/design-system-components Minor
@swisspost/design-system-components-angular-workspace Patch
@swisspost/design-system-components-react Minor
@swisspost/design-system-components-angular Minor
@swisspost/design-system-nextjs-integration Patch
@swisspost/design-system-styles Minor
@swisspost/design-system-tokens Minor
@swisspost/design-system-intranet-header Minor
@swisspost/design-system-icons Minor
@swisspost/design-system-migrations Minor
@swisspost/design-system-styles-primeng Minor
@swisspost/internet-header Patch
@swisspost/design-system-intranet-header-workspace Patch
@swisspost/design-system-styles-primeng-workspace Patch
@swisspost/design-system-intranet-header-showcase Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@swisspost-bot
Copy link
Contributor

swisspost-bot commented Nov 20, 2024

Related Previews

Copy link

sonarcloud bot commented Nov 20, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@veyaromain
Copy link
Contributor Author

@alizedebray do you now why i get this code smell? This h import is used by <slot>

@alizedebray
Copy link
Contributor

@alizedebray do you now why i get this code smell? This h import is used by <slot>

@veyaromain this is a false positive, I just accepted the issue on SonarCloud

Copy link
Contributor

@alizedebray alizedebray left a comment

Choose a reason for hiding this comment

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

Your code is very clean, well done! 👍
I've added a few change requests. If you need more details about anything, feel free to ask.

'@swisspost/design-system-components': minor
---

Added the `post-linkarea` component.`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Added the `post-linkarea` component.`
Added the `post-linkarea` component.

describe('default', () => {
beforeEach(() => {
cy.getComponent('linkarea', LINKAREA_ID);
cy.get('@linkarea').as('linkArea');
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is unnecessary. The .as('*') command is used to give an element an alias, so you can reference it later with @*. Therefore, this line is simply getting the @linkArea element and assigning it the same alias again, which is redundant.

Suggested change
cy.get('@linkarea').as('linkArea');

Comment on lines +31 to +33
cy.get('post-linkarea a[data-link]').should('exist');

cy.get('post-linkarea a[data-link]').then($link => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is a good place to use the .as() command:

Suggested change
cy.get('post-linkarea a[data-link]').should('exist');
cy.get('post-linkarea a[data-link]').then($link => {
cy.get('@linkArea').find('a[data-link]').as('linkAreaTarget');
cy.get('@linkAreaTarget').should('exist');
cy.get('@linkAreaTarget').then($link => {

@@ -0,0 +1,4 @@
:host {
display: block;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're not sure whether the element inside post-linkarea will be an inline or block element, it's better to set the wrapper to display: contents. This makes the post-linkarea behave as if it were replaced by its content in the DOM. Its logic remains intact, but it no longer acts as a box. Here’s an article that explains this well: https://la-cascade.io/articles/comment-fonctionne-css-display-content.

Suggested change
display: block;
display: contents;

Comment on lines +38 to +39
name: 'Default link',
description: 'This is the link that will be used if no specific link is set.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: 'Default link',
description: 'This is the link that will be used if no specific link is set.',
name: 'First link URL',
description: 'This is the URL used for the first link in the card.',

Comment on lines +48 to +50
name: 'Specified link',
description:
'This is the link that will be used if you manually assign one using the `data-link` option.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: 'Specified link',
description:
'This is the link that will be used if you manually assign one using the `data-link` option.',
name: 'Second link URL',
description: 'This is the URL used for the second link in the card.',

argTypes: {
dataLink: {
name: 'Custom selector',
description: 'Allows you to use a different link instead of the default first one.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: 'Allows you to use a different link instead of the default first one.',
description: 'If `false`, clicking the card redirects to the first link. If `true`, a `data-link` attribute is added to the second link, which is used instead, overriding the default behavior.',

<p class="card-text">Contentus momentus vero siteos et accusam iretea et justo.</p>

<a class="card-link" href="${args.anchorDefaultLink}">Ligilo teksto</a>
<a class="card-link" href="${args.anchorSepcifiedLink}" ${spread(props)}>Pli da ligo</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you can use nothing, when a attribute receives nothing then it is not rendered at all:

Suggested change
<a class="card-link" href="${args.anchorSepcifiedLink}" ${spread(props)}>Pli da ligo</a>
<a class="card-link" href="${args.anchorSepcifiedLink}" data-link=${args.dataLink ? '' : nothing}>Pli da ligo</a>

Then you no longer need the createSecondAnchorProps function.

export const Default: StoryObj<HTMLPostLinkareaElement> = {};

export const InitiallySpecified: StoryObj<HTMLPostLinkareaElement> = {
render: args => html` ${renderLinkarea({ ...args, dataLink: true })} `,
Copy link
Contributor

Choose a reason for hiding this comment

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

You can simply override the args like so:

Suggested change
render: args => html` ${renderLinkarea({ ...args, dataLink: true })} `,
args: {
dataLink: true,
}

@Component({
tag: 'post-linkarea',
styleUrl: 'post-linkarea.scss',
shadow: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

You do not need to use a shadow DOM for this component as it does not render additional elements (only the host and its content) and the styles are minimal.

Suggested change
shadow: true,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[component]: Link area
3 participants