feat: typed events #1981
feat: typed events #1981
Conversation
|
Thank you for creating a Pull Request @gavinbarron. This is a checklist for the PR reviewer(s) to complete before approving and merging this PR:
|
Changes the package use to generate the custom-elements.json file used to build React components and tell Storybook about the web-components definitions.
gavinbarron
force-pushed
the
feat/typed-events
branch
from
0ee3993
to
9566106
Compare
|
The updated storybook is available here |
|
This is great! There is a lot of noise between attributes and properties (some are there twice), there are also private members that seem to be documented. CSS properties are amazing to be documented, I think if we could add some context (especially when it uses another css property as its default value) would be appreciated. Events are great and help a lot. This also makes me thinks... Could we use this approach to document our permissions matrix? We do it on our docs, but it's not part of the code... Maybe it could be useful? I feel this requires a LOT of comments changes but would allow our documentation to always be up-to-date. One thought : How do we make sure we don't duplicate work with what we have on https://aka.ms/mgt/docs? If we could identify ways to get the content and produce a Markdown table that we could automatically PR to the docs, it would be great. |
|
The updated storybook is available here |
Attributes are the string inputs which map to properties, some of which convert from strings to other types, so the "duplication" is not unexpected or incorrect. Think of it this way, a developer working HTML + JavaScript can pass data via the attribute as a string, or if they get a reference to the DOM element, they can directly set the properties themselves. Developers using the React wrappers are directly setting the properties, so both have distinct value propositions. Re: private and protected items, that's something I think I can address by processing the data when we pass it to Storybook. The underlying cause here is that Storybook takes the opinion that only genuinely private members, i.e. those starting with at #, should be excluded. storybookjs/storybook#15436 Good idea on the CSS properties, let me see what we can do.
This is an interesting idea. One that we should explore if we move forward with this tooling and restore the docs page to operation. I suspect it might need us to build a plugin for the custom elements manifest analyser
For the events side of things it's work we 100% should do so that our event handlers have proper typings, not having those types available correctly in TypeScript is a miss on our part IMO. We can use this tooling to generate markdown too! https://github.com/open-wc/custom-elements-manifest/tree/master/packages/to-markdown |
|
The updated storybook is available here |
|
The updated storybook is available here |
|
The updated storybook is available here |
|
The updated storybook is available here |
|
LGTM! |
BREAKING CHANGE: moves event from a property of e.detail to be the value of e.detail
gavinbarron
dismissed
sebastienlevert’s stale review
The base branch was changed.
|
As there will be breaking changes in this it is being re-targeted to next/fluentui #2005 will bring next/fluentui up to date with main and thus this PR is dependent on 2005 |
BREAKING CHANGE: all events for mgt-task now emit a CustomEvent<ITask>
|
The updated storybook is available here |
|
The updated storybook is available here |
|
@gavinbarron should we bring back the Docs tab from Storybook as part of this PR? |
|
@sebastienlevert YES! I thought it was in here, but I guess not, fixing that now. |
|
The updated storybook is available here |
|
I can't find the Docs with typed information on |
|
Looks like there's an interaction between the disambiguation changes and the generation of the custom-elements.json file that is not working in our favor now... |
…ion to the data in custom-elements.json
|
The updated storybook is available here |
|
@sebastienlevert given the number of challenges with the docs page, I'm going to suggest that we leave it disabled in this PR and introduce another separate change to review that improvement separately as there are a lot of issues to resolve before we're ready for showtime with that feature |
disable docs page again set code pane content on agenda disable ArgsTable on docs pages by default show ArgsTable on base agenda stories
|
The updated storybook is available here |
|
The updated storybook is available here |
|
The updated storybook is available here |
Closes #1689
PR Type
Description of the changes
Changes the package use to generate the custom-elements.json file used to build React components and tell Storybook about the web-components definitions.
A couple of thing to consider here. the format being used for the custom-elements.json is very different in v1.0 vs the experimental output we were previously using. The lit support is there, but currently there's no way to differentiate between properties of a class vs those properties which are decorated so that lit tracks them as reactive properties to trigger re-renders.
I've made a guess at how we should be doing this by settings conditions in the new version of generate.js which writes our react.ts file. The result now is that we see two new properties being added to the Get React component and the config property being removed from the Person component. IMO as the config is a static property it doesn't make much sense to have this exposed via a React Component as changing it on one component is changing it globally,
I'll need to test if React devs still have access to the static property on the MgtPerson class to ensure this is still usable.
WRT the response and error properties on Get, IMO these should probably be internal properties and private as I don't see them needing to be accessible from outside of the component.
PR checklist
yarn build) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)yarn setLicense)Other information
Storybook still works. We should evaluate the data on the docs page between the current version and what this change provides.
If we decide that this is a useful feature the other fires jsdoc comments will need to be updated with typings