-
Notifications
You must be signed in to change notification settings - Fork 308
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
chore(content-explorer): Migrate shareDialog #3735
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need the flow files for the internal components of ContentExplorer? shouldn't the flow only be needed for the outer component that consumers use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think we should leave it out? its only there because i ran the codemod on it
type="text" | ||
value={url} | ||
/> | ||
<Button autoFocus className="be-modal-button-copy" onClick={copy} variant="primary"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does button have autoFocus
prop?
tsconfig.json
Outdated
@@ -15,5 +15,6 @@ | |||
"strict": false | |||
}, | |||
"files": [".storybook/typings.d.ts"], | |||
"include": ["src/**/*.ts", "src/**/*.tsx", "examples/src/**.tsx", "types.ts", "scripts/jest/jest-setup.ts"] | |||
"include": ["src/**/*.ts", "src/**/*.tsx", "examples/src/**.tsx", "types.ts", "scripts/jest/jest-setup.ts"], | |||
"exclude": ["src/**/*.stories.ts", "src/**/*.stories.tsx"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we want to exclude stories?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it dawned on me that components and elements in stories do not need all the required props to demonstrate specific functions.
> | ||
<div className="be-modal-content"> | ||
<Text as="label"> | ||
<FormattedMessage tagName="div" {...messages.shareDialogText} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need tagName
still? i assume it was originally added for some block level style but i'm not sure if it's necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
going to remove this that should be there
89360b1
to
54392aa
Compare
1728cfc
to
b8c0428
Compare
b8c0428
to
7d4a0ad
Compare
before:
after: