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(app): display inputsignals as inputs rather than properties #1439

Merged
merged 2 commits into from
Mar 14, 2024

Conversation

astrutz
Copy link
Contributor

@astrutz astrutz commented Feb 23, 2024

Closes #1434.

I didn't find a better way to extract the input signals than with a regex inside the properties

@vogloblinsky
Copy link
Contributor

Did you know why tests are failing ?

@astrutz
Copy link
Contributor Author

astrutz commented Feb 25, 2024

No, I'll have a look on Thursday

@astrutz
Copy link
Contributor Author

astrutz commented Feb 29, 2024

@vogloblinsky I fixed it, forgot a condition for undefined. e2e seemed to fail because of a timeout and the 16.x ubuntu failed because of sonarcloud. But I don't think it is related to my feature

@astrutz
Copy link
Contributor Author

astrutz commented Mar 4, 2024

@vogloblinsky do you take it from here? The time I got from my company for Compodoc is used up now, but feel free to contact me if there are any questions

@vogloblinsky vogloblinsky merged commit 9710dab into compodoc:develop Mar 14, 2024
3 of 5 checks passed
@vogloblinsky
Copy link
Contributor

Thanks for the PR

@valentinpalkovic
Copy link
Contributor

Hi @vogloblinsky

Storybook core maintainer here 👋

Do you have a rough estimate of when the input/output signal work will be released? We have made the necessary adjustments in Storybook to support both signal types type-wise. We await a Compodoc release to announce full signal support in Storybook.

@vogloblinsky
Copy link
Contributor

This week or beggining of next week.

@vogloblinsky vogloblinsky added this to the 1.1.24 milestone Apr 12, 2024
public getInputSignals(props) {
let inputSignals = [];
props?.forEach((prop, i) => {
const regexp = /input(?:\.(required))?(?:<([\w-]+)>)?\(([\w-]+)?\)/;
Copy link
Contributor

Choose a reason for hiding this comment

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

@astrutz This regex has quite some limitations. I will list the most obvious ones and potentially we can adjust the regex to cover the cases:

  • input("helloworld") -> strings are not supported as default values
  • input(false, { transform: booleanAttribute }); -> as soon as a transformation option is used, the regex doesn't match

I am thinking of potentially rather use some babel parsing here instead of relying on a regex, since the expression might also have some new line characters, especially if the options object is used to provide a transform function

Copy link
Contributor

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

@vogloblinsky thank you so much for this if you can support the cases @valentinpalkovic mentioned, thank you so much in advance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@valentinpalkovic I agree with you, although I can't really do much, as I don't have the capacity atm. I'll check with company and can come back to you in about 3 weeks

Copy link

@tobiaskau tobiaskau Jun 12, 2024

Choose a reason for hiding this comment

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

@astrutz Is there a Github issue ticket for that? This issue in relation to Storybook is quite important to me currently as it unfortunately makes it kind of unusable currently :(

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.

[FEATURE] Support 17.1 input signals
5 participants