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(pie-button): DSW-1539 wip assigning size prop to an icon slot #1128

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

Conversation

dandel10n
Copy link
Contributor

@dandel10n dandel10n commented Dec 22, 2023

Describe your changes (can list changeset entries if preferable)

The idea:

Currently to make sure that when we provide an icon slot in our web components we use css variables to ensure the correct icon size. This approach works but goes against size property used on icon component. The goal of this change is to assign size icon prop to the icon slot to make sure that the size prop is correct. We can still use a css var for the option when consumers pass custom svg instead of pie icon.

Problems:

  • SSR:
    • there is an issue with webc icons package to be fixed first: https://justeattakeaway.atlassian.net/browse/DSW-1686
    • Tested with Tag component inside the button as a POC while icons don't render in SSR:
      • the logic works in vanilla js app when js is enabled;
      • Then I checked Next app and while with yarn dev it worked the same way as in vanilla js, it didn't work as I would expect even without ssr after yarn build && yarn preview. While the attr was attached to the top wrapper it was not passed down to shadow dom. And then with disabled js the attr was not attached even to the top level element :sadness:

Vanilla JS:

Screenshot 2024-04-12 at 16 06 05

NextJS prod build served locally when js is enabled:

Screenshot 2024-04-12 at 16 21 07

NextJS when js is disabled:

Screenshot 2024-04-12 at 16 02 06

Copy link

changeset-bot bot commented Dec 22, 2023

🦋 Changeset detected

Latest commit: cf952f7

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

This PR includes changesets to release 14 packages
Name Type
@justeattakeaway/pie-button Minor
wc-angular12 Patch
wc-next10 Patch
wc-next13 Patch
wc-nuxt2 Patch
wc-nuxt3 Patch
wc-react17 Patch
wc-react18 Patch
wc-vanilla Patch
wc-vue3 Patch
pie-storybook Patch
@justeattakeaway/pie-cookie-banner Patch
@justeattakeaway/pie-modal Patch
@justeattakeaway/pie-webc 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

@xander-marjoram
Copy link
Contributor

/snapit testing latest changes

@pie-design-system-bot
Copy link
Contributor

Starting a new snapshot build. You can view the logs here.

@pie-design-system-bot
Copy link
Contributor

@xander-marjoram Your snapshots have been published to npm!

Test the snapshots by updating your package.json with the newly-published versions:

Note

If you have more than one of these packages installed, we suggest using the new snapshots for all of them to help avoid version conflicts.

yarn up @justeattakeaway/[email protected] --mode=update-lockfile
yarn up @justeattakeaway/[email protected] --mode=update-lockfile
yarn up @justeattakeaway/[email protected] --mode=update-lockfile
yarn up @justeattakeaway/[email protected] --mode=update-lockfile
yarn up @justeattakeaway/[email protected] --mode=update-lockfile
yarn up @justeattakeaway/[email protected] --mode=update-lockfile

Then finally:

yarn install

@dandel10n dandel10n force-pushed the dsw-1539-setting-props-to-a-slot branch from fba2a3a to 466bf31 Compare April 12, 2024 11:54
@dandel10n
Copy link
Contributor Author

/snapit

@pie-design-system-bot
Copy link
Contributor

Starting a new snapshot build. You can view the logs here.

@pie-design-system-bot
Copy link
Contributor

No changed packages found! Please make sure you have added a changeset entry for the packages you would like to snapshot.

@github-actions github-actions bot temporarily deployed to pie-storybook-pr-1128 April 12, 2024 12:01 Inactive
@dandel10n dandel10n force-pushed the dsw-1539-setting-props-to-a-slot branch from 466bf31 to cf952f7 Compare April 12, 2024 12:13
@dandel10n
Copy link
Contributor Author

/snapit

@pie-design-system-bot
Copy link
Contributor

Starting a new snapshot build. You can view the logs here.

@pie-design-system-bot
Copy link
Contributor

@dandel10n Your snapshots have been published to npm!

Test the snapshots by updating your package.json with the newly-published versions:

Note

If you have more than one of these packages installed, we suggest using the new snapshots for all of them to help avoid version conflicts.

yarn up @justeattakeaway/[email protected] --mode=update-lockfile
yarn up @justeattakeaway/[email protected] --mode=update-lockfile
yarn up @justeattakeaway/[email protected] --mode=update-lockfile
yarn up @justeattakeaway/[email protected] --mode=update-lockfile

Then finally:

yarn install

@github-actions github-actions bot temporarily deployed to pie-storybook-pr-1128 April 12, 2024 12:19 Inactive
@dandel10n
Copy link
Contributor Author

/snapit

@pie-design-system-bot
Copy link
Contributor

Starting a new snapshot build. You can view the logs here.

@pie-design-system-bot
Copy link
Contributor

@dandel10n Your snapshots have been published to npm!

Test the snapshots by updating your package.json with the newly-published versions:

Note

If you have more than one of these packages installed, we suggest using the new snapshots for all of them to help avoid version conflicts.

yarn up @justeattakeaway/[email protected] --mode=update-lockfile
yarn up @justeattakeaway/[email protected] --mode=update-lockfile
yarn up @justeattakeaway/[email protected] --mode=update-lockfile
yarn up @justeattakeaway/[email protected] --mode=update-lockfile

Then finally:

yarn install

@github-actions github-actions bot temporarily deployed to pie-storybook-pr-1128 April 12, 2024 13:46 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants