Angular: Allow usage of all component valid selectors #14230
Conversation
shilman
changed the title
|
Could I have some feedback on the unsuccessful checks please so I can ensure future PR's are successful. Thanks |
|
@dexster don't worry about the unsuccessful checks 👍 |
| const buildTemplate = ( | ||
| selector: string | ||
| ): { | ||
| openTag?: string; |
There was a problem hiding this comment.
Do you think it would be possible to have openTag and closeTag not optional ?
There was a problem hiding this comment.
N/A with new commit using the regex you mentioned
| }; | ||
| } | ||
| return acc; | ||
| }, {}); |
There was a problem hiding this comment.
what do you think about starting with a default value?
like storybook-default-selector ? (or something like that)
In case of an error it could be a workaround?
or we could display a clearer error if the open/close tag is not found ?
There was a problem hiding this comment.
N/A with new commit using the regex you mentioned which should always return a selector.
| const templates = [ | ||
| { | ||
| // Match element selectors with optional chained attributes or classes | ||
| re: /^([\w\d-_]+)(?:(?:\[([\w\d-_]+)(?:=(.+))?\])|\.([\w\d-_]+))?/, |
There was a problem hiding this comment.
a long time ago when I was doing Jquery I used this solution (regex) : https://codegolf.stackexchange.com/a/123281
I don't know if you made them yourself but maybe this could improve or simplify 🤷♂️ (not sure)
There was a problem hiding this comment.
Done with modifications
|
|
||
| @Component({ | ||
| selector: 'storybook-attribute-selector[foo]', | ||
| template: '<p>Attribute selector</p>', |
There was a problem hiding this comment.
I find these examples very interesting.
We could display the html output expected in template 🤔 like :
template: `
<h3>Attribute selector</h3>
Selector: "storybook-attribute-selector[foo]" <br>
Generated template: "<storybook-attribute-selector foo></storybook-attribute-selector>"
`,but maybe it's too much 🤷♂️
|
In angular it is valid to have multiple selectors for the same directive/component. How do we handle that? In that case we probably want to be able to choose which selector we want to use. |
|
For the purposes of Storybook auto-generated templates I simply use the first selector. If someone wanted to use one of the other selectors from their component I would expect them to use the template property and specify the required selector. |
|
Changes made as per feedback |
|
@dexster @ThibaudAV is this good to merge? if we're going to include it in 6.2 we should probably merge it today. (doesn't mean we can't get it in later, or as a patch, but ideally we'd wrap everything ASAP). thanks! |
|
I've made all the updates as per @ThibaudAV recommendations so happy from my side. |
|
Thanks @dexster 🙏 merging and will release later today! |
|
Coool thanks @dexster 🙂 |
Issue: #14079
What I did
Added checks for all valid angular directive/component selectors to allow templates to be generated correctly.
How to test
Is this testable with Jest or Chromatic screenshots? Yes
Tests added
Does this need a new example in the kitchen sink apps?
Possibly. Examples added but they don't show much. Could be removed?
Does this need an update to the documentation?
No