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

fix(components): remove scss module import #3358

Merged
merged 3 commits into from
Aug 13, 2024
Merged

fix(components): remove scss module import #3358

merged 3 commits into from
Aug 13, 2024

Conversation

alizedebray
Copy link
Contributor

No description provided.

@alizedebray alizedebray requested a review from a team as a code owner July 30, 2024 15:02
@alizedebray alizedebray linked an issue Jul 30, 2024 that may be closed by this pull request
Remove card control scss import #3357
Closed
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jul 30, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 37f7c97

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-components Patch
@swisspost/design-system-components-angular-workspace Patch
@swisspost/design-system-components-react Patch
@swisspost/design-system-documentation Patch
@swisspost/design-system-components-angular Patch
@swisspost/design-system-nextjs-integration Patch
@swisspost/design-system-styles Patch
@swisspost/design-system-intranet-header Patch
@swisspost/design-system-icons Patch
@swisspost/design-system-migrations Patch
@swisspost/design-system-styles-primeng Patch
@swisspost/design-system-demo Patch
@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 Jul 30, 2024 •
edited
Loading

Related Previews

@alizedebray alizedebray requested a review from gfellerph July 30, 2024 15:02
@alizedebray alizedebray mentioned this pull request Jul 31, 2024
Open
@alizedebray alizedebray self-assigned this Jul 31, 2024
@gfellerph
Copy link
Member

I somehow doubt that both solutions (the :export and the new hostContext patch) are the right approach to solve the underlying problem (which I don't really understand from context here). Web components should be built context independent and if the context has an influence on how the component is rendered, the context should be responsible of doing the customization while the web component just offers the possibility to customize what's needed through options.

TLDR; no component should change based on context, the context should change the component.

@gfellerph gfellerph removed their request for review August 12, 2024 11:54
@oliverschuerch
Copy link
Contributor

oliverschuerch commented Aug 12, 2024 •
edited
Loading

I somehow doubt that both solutions (the :export and the new hostContext patch) are the right approach to solve the underlying problem (which I don't really understand from context here). Web components should be built context independent and if the context has an influence on how the component is rendered, the context should be responsible of doing the customization while the web component just offers the possibility to customize what's needed through options.

TLDR; no component should change based on context, the context should change the component.

  1. As far as I understand, the solution provided in this PR is doing the same thing as before (with some small updates), just with another technic. Therefore I'm not sure if this is necessary? @alizedebray please correct me if I'm wrong.
  2. @gfellerph I totally understand your statement. But the question is where it comes from, when it was made and for what?
    In my opinion, this is correct when it comes to different logical behaviour and or a totally different visuality, so a user would not recognise the component anymore. But I don't agree with this statement when it comes to small visual updates like different colors to provide good contrast.
  3. The :host-context() selector will eventually never be implemented by all browsers. Read about it here.
    I feel the same as the author of this article and in addition to his thoughts, I don't really see the difference between a media query and the :host-context() selector. Why should it be totally fine to use media queries to switch a components visuality, but it's called an anti-pattern, when we want to use :host-context() to react to a background-color which is not defined within the component itself. The styles for that need to be implemented somewhere and I totally prefer to have them inside of the component itself, than scattered in many different files.
    This does not mean, I'm not open for another solution. I only think it's better to define things where they belong.

@alizedebray
Copy link
Contributor Author

  1. As far as I understand, the solution provided in this PR is doing the same thing as before (with some small updates), just with another technic. @alizedebray please correct me if I'm wrong.

@oliverschuerch Exact. :export cannot be used with the 'dist-custom-elements' output target so I created this PR as a workaround to fix the bug while keeping your initial logic.

@alizedebray
Copy link
Contributor Author

I somehow doubt that both solutions (the :export and the new hostContext patch) are the right approach to solve the underlying problem (which I don't really understand from context here).

@gfellerph I completely agree and we should no longer have this issue if we implement the component tokens right. This PR fixes to build in the meantime, giving us time to come with a better solution.

oliverschuerch
oliverschuerch approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@oliverschuerch oliverschuerch 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 a changeset

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@alizedebray alizedebray merged commit be80b68 into main Aug 13, 2024
12 checks passed
@alizedebray alizedebray deleted the 3357-remove-card-control-scss-import branch August 13, 2024 13:22
@swisspost-bot swisspost-bot mentioned this pull request Aug 13, 2024
Merged
gfellerph pushed a commit that referenced this pull request Aug 22, 2024
@swisspost-bot @github-actions
chore(changesets): 🦋📦 publish packages (main) (#3374)
0667bd7 
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @swisspost/[email protected]

### Minor Changes

- Added the post-logo component, which enables displaying the Post's
logo either as a clickable link or as a simple image. (by
[@alizedebray](https://github.com/alizedebray) with
[#3354](#3354))

### Patch Changes

- Fixed the `post-card-control` component to use the correct color
scheme when placed on nested colored backgrounds. (by
[@alizedebray](https://github.com/alizedebray) with
[#3358](#3358))
-   Updated dependencies:
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Minor Changes

- Added the post-logo component, which enables displaying the Post's
logo either as a clickable link or as a simple image. (by
[@alizedebray](https://github.com/alizedebray) with
[#3354](#3354))

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Minor Changes

- Added icon number 2612. (by
[@swisspost-bot](https://github.com/swisspost-bot) with
[#3373](#3373))

## @swisspost/[email protected]

### Minor Changes

-   Added four new entry files that enable working with Design Tokens:
    -   post-external.(s)css: For portal and other external pages
    -   post-internal.(s)css: For applications and other internal pages
    -   post-tokens-external.(s)css: External tokens only
- post-tokens-internal.(s)css: Internal tokens only (by
[@gfellerph](https://github.com/gfellerph) with
[#3349](#3349))

### Patch Changes

- Realigned the checkbox and the radio button with the label. (by
[@davidritter-dotcom](https://github.com/davidritter-dotcom) with
[#3366](#3366))

- Set the `max-width` constraint of the tag component to 100% for
improved accessibility. Try to keep tag text as short as possible
though. (by [@gfellerph](https://github.com/gfellerph) with
[#3388](#3388))

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]

## @swisspost/[email protected]



## @swisspost/[email protected]

### Minor Changes

- Added the post-logo component, which enables displaying the Post's
logo either as a clickable link or as a simple image. (by
[@alizedebray](https://github.com/alizedebray) with
[#3354](#3354))

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Minor Changes

- Added the post-logo component, which enables displaying the Post's
logo either as a clickable link or as a simple image. (by
[@alizedebray](https://github.com/alizedebray) with
[#3354](#3354))

### Patch Changes

- Fixed the font in the full page previews. (by
[@alizedebray](https://github.com/alizedebray) with
[#3378](#3378))

- Marked the card button and the carousel as deprecated (will be removed
in a future version). (by [@schaertim](https://github.com/schaertim)
with [#3380](#3380))

-   Updated dependencies:
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]
    -   @swisspost/[email protected]

## @swisspost/[email protected]

### Patch Changes

-   Updated dependencies:
    -   @swisspost/[email protected]

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliverschuerch oliverschuerch oliverschuerch approved these changes

Assignees

@alizedebray alizedebray

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove card control scss import
4 participants
@alizedebray @swisspost-bot @gfellerph @oliverschuerch