Adds more properties #1
Conversation
|
Awesome! |
| @@ -1 +1,2 @@ | |||
| node_modules | |||
| package-lock.json | |||
There was a problem hiding this comment.
Any particular reason why you are ignoring the package-lock.json?
Was under the impression that this would give us a deterministic install of the project.
There was a problem hiding this comment.
You might be right. I am used to always ignore package-lock since it is auto-generated and I am thinking to have one source of the truth. But npm itself does recommend committing the file.
| import { SpectrumComponentSize } from './common'; | ||
| declare namespace Spectrum { | ||
| type IconName = 'ui:AlertMedium' | 'ui:AlertSmall' | 'ui:ArrowDownSmall' | 'ui:ArrowLeftMedium' | 'ui:ArrowUpSmall' | 'ui:Asterisk' | 'ui:CheckmarkMedium' | 'ui:CheckmarkSmall' | 'ui:ChevronDownMedium' | 'ui:ChevronDownSmall' | 'ui:ChevronLeftLarge' | 'ui:ChevronLeftMedium' | 'ui:ChevronRightLarge' | 'ui:ChevronRightMedium' | 'ui:ChevronRightSmall' | 'ui:ChevronUpSmall' | 'ui:CornerTriangle' | 'ui:CrossLarge' | 'ui:CrossMedium' | 'ui:CrossSmall' | 'ui:DashSmall' | 'ui:DoubleGripper' | 'ui:FolderBreadcrumb' | 'ui:HelpMedium' | 'ui:HelpSmall' | 'ui:InfoMedium' | 'ui:InfoSmall' | 'ui:Magnifier' | 'ui:More' | 'ui:SkipLeft' | 'ui:SkipRight' | 'ui:Star' | 'ui:StarOutline' | 'ui:SuccessMedium' | 'ui:SuccessSmall' | 'ui:TripleGripper'; | ||
| type IconSize = 'xxs' | 'xs' | 's' | 'm' | 'l' | 'xl' | 'xxl'; |
There was a problem hiding this comment.
Do we lose icon sizes by this being replaced with common?
Do more icon sizes exist than do common sizes?
There was a problem hiding this comment.
Somebody in Slack reported this as a "bug". I did not test it but I blindly trust it. :-D
dist/MenuItem.d.ts
Outdated
| 'sp-menu-item': { | ||
| children?: React.ReactNode; | ||
| ref?: React.RefObject<HTMLElement>; | ||
| key?: string; |
There was a problem hiding this comment.
Good call on adding key to the list types!
There was a problem hiding this comment.
Yes, a few months ago I had to add "key" to the menu item component in order to update its state correctly. But we can't simply have "key" property on the component since it will be undefined. We have to find a better way.
There was a problem hiding this comment.
The beauty of React here is that key={undefined} will actually not render a key at all!
I use the same trick for making sure false doesn't add an attribute to the web component when the spec calls for not rendering the attribute.
Alternatively, we could also generate keys if it is not passed into the React Component to silence warnings about missing keys in lists...
Maybe I should whip up some tests around these missing keys... 🤔
You should doublecheck this with PS 22.5.0 |
|
I noticed you had docs added to the PR. Was that because of the absence of a .gitignore, or do you prefer to have generated docs committed in the repo? Should I add a .gitignore for the docs, or check the generated docs in? Could add a build step to just deploy the docs to GitHub pages upon commit/merge. |
|
ah, I was just trying to figure out on how to build usable types and wasn't sure whether I should keep it or not. I don't see docs as valuable right now in my case. VSCode and intelli-sense can show you hints as well including your own descriptions. |
|
Will these changes be merged anytime soon? |
|
@valentinsenm looks like this PR has merge conflicts at the moment. If I get a clean PR happy to merge. I canceled my Adobe subscription and won't be able to validate the changes locally. If anyone is up for maintaining or pushing this repo forward, I will happily transfer ownership. |
|
I would rather push on the folk in Adobe to provide official types. I don't want to maintain this either :-D |
4 participants
It is not perfect but works for me.