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

Web-components: Fix custom-elements order of property application #19183

Conversation

sonntag-philipp
Copy link

Issues:

What I did

I updated the order in which the different declaration information is extracted from the manifest. Previously, the members would override all attributes which would make attributes less specifically shown as "properties" in the storybook. This makes understanding the implementation harder.

Furthermore, I updated the sb-button example with a prefix slot that can be used to show the slot capabilities of the Storybook. This allows users to set a string value as content for for a slot within a component.
To enable this functionality I had to add the slots declaration type to the category evaluation. In old examples, the type of the slot would default to void which would not allow the user to change it's value. This is now changed to string as default for slots, as they don't define a type themselves in the manifest.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

Philipp Sonntag added 2 commits September 14, 2022 10:02
@ndelangen
Copy link
Member

Hello @sonntag-philipp Thank you for contributing to storybook!

I'm trying to merge in next into this branch but GitHub is preventing me from doing it:

(sonntag-philipp/issues-19167-18858-custom-elements-manifest-update) % git push                                                                                        ~/Projects/Storybook/core/code
Enumerating objects: 52, done.
Counting objects: 100% (52/52), done.
Delta compression using up to 10 threads
Compressing objects: 100% (18/18), done.
Writing objects: 100% (18/18), 2.06 KiB | 2.06 MiB/s, done.
Total 18 (delta 17), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (17/17), completed with 17 local objects.
To https://github.com/sonntag-philipp/storybook.git
 ! [remote rejected]       sonntag-philipp/issues-19167-18858-custom-elements-manifest-update -> sonntag-philipp/issues-19167-18858-custom-elements-manifest-update (refusing to allow an OAuth App to create or update workflow `.github/workflows/generate-repros-next.yml` without `workflow` scope)
error: failed to push some refs to 'https://github.com/sonntag-philipp/storybook.git'

I don't think there's anything you or I can do to remedy that situation.. but could you merge in the next branch from the upstream remote? 🙏 thanks!

@ndelangen
Copy link
Member

@sonntag-philipp We're also going to be removing the web-components example soon-ish.

We're moving towards a testing strategy that has shared generic stories (so we don't have duplicated tests).
Any web-components specific stories will have to be written here: code/renderers/web-components/template in the future.

Would you be able to help us move these stories over?

@sonntag-philipp
Copy link
Author

Thank you for the feedback. I will merge next asap, but I don't think it will be within the next days.

I would really like to help, but I don't have much time right now, sorry.

@shilman shilman added the linear label Oct 27, 2022
@shilman
Copy link
Member

shilman commented Oct 27, 2022

I tried moving @sonntag-philipp 's example over to our sandboxes but it isn't working. As far as I can tell his code is working, but the example isn't. I'll see if I can pair with @kasperpeulen to figure it out.

@ndelangen ndelangen requested a review from a team January 13, 2023 22:37
@ndelangen
Copy link
Member

@storybookjs/web-components is this good to merge?

@yannbf
Copy link
Member

yannbf commented Apr 19, 2023

@shilman @kasperpeulen tagging you as you meant to take a look at this. I updated the branch with the latest next

default:
type = { name: 'void' };
break;
}
Copy link
Contributor

@usrrname usrrname Apr 29, 2023

Choose a reason for hiding this comment

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

This is awesome, and sorely needed! I'd contemplate bringing this into @storybook/html as well re: transformation of attributes as opposed to properties to args. I say this because it's an easy conflation to newcomers and having a way to distinguish these... would make onboarding to WC DS much easier.

@shilman shilman changed the title custom-elements: Changed order of property application and updated example with slot Web-components: Fix custom-elements order of property application Jun 9, 2023
@shilman shilman requested a review from a team as a code owner June 9, 2023 03:27
@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jun 9, 2023
@usrrname
Copy link
Contributor

usrrname commented Jun 9, 2023

WOOHOOOO

@shilman
Copy link
Member

shilman commented Jun 9, 2023

@usrrname better late than never 🙈

@github-actions github-actions bot mentioned this pull request Jun 13, 2023
69 tasks
shilman added a commit that referenced this pull request Jun 13, 2023
…-19167-18858-custom-elements-manifest-update

Web-components: Fix custom-elements order of property application
(cherry picked from commit abfc677)
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jun 13, 2023
shilman added a commit that referenced this pull request Jun 13, 2023
…-19167-18858-custom-elements-manifest-update

Web-components: Fix custom-elements order of property application
(cherry picked from commit abfc677)
kasperpeulen pushed a commit that referenced this pull request Jun 13, 2023
…-19167-18858-custom-elements-manifest-update

Web-components: Fix custom-elements order of property application
(cherry picked from commit abfc677)
@shilman shilman removed the patch:done Patch/release PRs already cherry-picked to main/release branch label Jun 14, 2023
shilman added a commit that referenced this pull request Jun 14, 2023
…-19167-18858-custom-elements-manifest-update

Web-components: Fix custom-elements order of property application
(cherry picked from commit abfc677)
kasperpeulen pushed a commit that referenced this pull request Jun 14, 2023
…-19167-18858-custom-elements-manifest-update

Web-components: Fix custom-elements order of property application
(cherry picked from commit abfc677)
kasperpeulen pushed a commit that referenced this pull request Jun 14, 2023
…-19167-18858-custom-elements-manifest-update

Web-components: Fix custom-elements order of property application
(cherry picked from commit abfc677)
@JReinhold JReinhold added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch web-components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants