PD-567: IconButton accessibility, TS fixes #352
Conversation
package.json
Outdated
| @@ -35,7 +35,7 @@ | |||
| "prettier:check": "prettier --check \"**/*.{js,ts,tsx,json,md}\"", | |||
| "test": "jest --env jsdom", | |||
| "ts": "tsc --build tsconfig.json", | |||
| "prepublishOnly": "yarn lint && yarn test && yarn build", | |||
| "prepublishOnly": "yarn lint && yarn test && yarn build && yarn ts", | |||
There was a problem hiding this comment.
Previously, only new JS files would be built as a part of the pre-publish script, leaving potentially (though unlikely) out of date TS files in the dist directory of packages being published.
There was a problem hiding this comment.
Should we swap the order of this then? Cuz yarn lint already runs yarn ts.
There was a problem hiding this comment.
Whoops, seemed to have forgotten about that. Should we actually have lint run tsc without emitting declaration files? It seems like a very side effect-y thing to be doing for a linting script.
There was a problem hiding this comment.
If we can do that easily then that WFM but it may be tricky to actually pull off.
Might be easier to just remove it from lint entirely and have it be its own individual task that we call individually?
There was a problem hiding this comment.
I tried just passing --noEmit, but that didn't work out the box. I wanna keep this on our radar as an improvement we could make, because I feel pretty strongly relying on a lint script to build part of our packages is a bad idea. I'll make a ticket.
There was a problem hiding this comment.
DesignerDave
changed the title
|
Size Change: +559 B (0%) Total Size: 530 kB
ℹ️ View Unchanged
|
Lol at the warning for +305 Bytes |
packages/lib/src/index.ts
Outdated
| export type Either<T, Keys extends keyof T = keyof T> = Omit<T, Keys> & | ||
| { | ||
| [K in Keys]-?: Required<Pick<T, K>> & | ||
| Partial<Record<Exclude<Keys, K>, undefined>>; |
There was a problem hiding this comment.
Shouldn't this be Partial<Pick<T, Exclude<Keys, K>>>? At least, I'm pretty sure that would match your example. As it is, if I'm reading it right, this would be equivalent to
interface FirstIsRequired extends SharedInExampleInterface {
sometimesRequired: boolean;
requiredOtherTimes?: undefined;
}with undefined instead of boolean. Of course, if the goal is to disallow people from providing both keys, then this is right and the example should be updated.
There was a problem hiding this comment.
Hmm... I think you're right here – good catch! I tested, and confirmed this change works. As is, it was requiring that only one of the two was passed when really having both is okay.
There was a problem hiding this comment.
Just pushed the fix 😁
| */ | ||
| export type Either<T, Keys extends keyof T = keyof T> = Omit<T, Keys> & | ||
| { | ||
| [K in Keys]-?: Required<Pick<T, K>> & Partial<Pick<T, Exclude<Keys, K>>>; |
There was a problem hiding this comment.
So just so I understand... the -?: here is removing the optionality from keyof T that's being "looped" through here, not from any of the keys that are actually output, right? Essentially just avoiding having an additional | undefined in the generated type.
There was a problem hiding this comment.
That's my understanding. In all fairness, this is a modified version of an answer in this SO thread: https://stackoverflow.com/questions/40510611/typescript-interface-require-one-of-two-properties-to-exist
I didn't know what -? did in TS until I found this hah
package.json
Outdated
| @@ -35,7 +35,7 @@ | |||
| "prettier:check": "prettier --check \"**/*.{js,ts,tsx,json,md}\"", | |||
| "test": "jest --env jsdom", | |||
| "ts": "tsc --build tsconfig.json", | |||
| "prepublishOnly": "yarn lint && yarn test && yarn build", | |||
| "prepublishOnly": "yarn lint && yarn test && yarn build && yarn ts", | |||
There was a problem hiding this comment.
Should we swap the order of this then? Cuz yarn lint already runs yarn ts.
|
Did you forget to push? I'm not seeing changes you said you made? |
Respond to PR feedback
|
Sorry Harry, I've been going through the comments as I address them locally. I'll re-request when I push. |
|
AKA now |
* changes * checkpoint * fix TS issue * add tests for title behavior * changesets * format * fix Either issue preventing multiple of the optional keys to be passed to the type * respond to PR feedback * remove additional ts build from prepublish script Respond to PR feedback
✍️ Proposed changes
Either<>that constructs a type that requires one of a set of optional keys in an interface.ariaLabelto the HTMLaria-label, and requires that eitheraria-labeloraria-labelledbyis present on the IconButton component.sizeisn't explicitly set on an Icon within IconButton, IconButton will now set the icon's size according to its own size prop..d.tsfiles in Icon and Portal pointing to types in a Storybook package. This was a problem in TypeScript apps that don't already use Storybook.🎟 Jira ticket: Apply accessibility attributes correctly to Icons and Icon Buttons
🛠 Types of changes
✅ Checklist
yarn changesetand documented my changes