-
Notifications
You must be signed in to change notification settings - Fork 8
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: cf952f7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
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 |
4969ff8
to
a03c4de
Compare
/snapit testing latest changes |
Starting a new snapshot build. You can view the logs here. |
@xander-marjoram Your snapshots have been published to npm! Test the snapshots by updating your 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 |
fba2a3a
to
466bf31
Compare
/snapit |
Starting a new snapshot build. You can view the logs here. |
No changed packages found! Please make sure you have added a changeset entry for the packages you would like to snapshot. |
466bf31
to
cf952f7
Compare
/snapit |
Starting a new snapshot build. You can view the logs here. |
@dandel10n Your snapshots have been published to npm! Test the snapshots by updating your 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 |
/snapit |
Starting a new snapshot build. You can view the logs here. |
@dandel10n Your snapshots have been published to npm! Test the snapshots by updating your 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 |
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:
yarn dev
it worked the same way as in vanilla js, it didn't work as I would expect even without ssr afteryarn 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:
NextJS prod build served locally when js is enabled:
NextJS when js is disabled: