-
Notifications
You must be signed in to change notification settings - Fork 918
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
Extract the button component for datasource picker to avoid duplicate code #6559
Extract the button component for datasource picker to avoid duplicate code #6559
Conversation
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
8f8f2c2
to
615fec3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6559 +/- ##
===========================================
+ Coverage 32.93% 67.80% +34.86%
===========================================
Files 2260 3411 +1151
Lines 45769 66740 +20971
Branches 7200 10860 +3660
===========================================
+ Hits 15075 45251 +30176
+ Misses 29984 18845 -11139
- Partials 710 2644 +1934
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
d2efc74
to
88ddd0d
Compare
88ddd0d
to
959984d
Compare
@@ -156,7 +157,7 @@ export class DataSourceView extends React.Component<DataSourceViewProps, DataSou | |||
if (this.state.showError) { | |||
return <DataSourceErrorMenu />; | |||
} | |||
const label = this.state.selectedOption.length > 0 ? this.state.selectedOption[0].label : ''; | |||
const label = this.state.selectedOption.length > 0 ? this.state.selectedOption[0].label! : ''; |
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.
Wouldn't label
be undefined here at initial render of the component if consumers pass in [{id}]
?
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.
Good catch! Let me add check here
f828569
to
878cecd
Compare
onClick: () => void; | ||
} | ||
|
||
export const PopoverButton: React.FC<PopoverButtonProps> = ({ className, label, onClick }) => { |
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.
Can we use a more specific name, i.e. DataSourceMenuButton
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.
Would do
return ( | ||
<> | ||
<EuiButtonEmpty | ||
className={`dataSourceComponentButtonTitle`} |
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.
can we use the className passed down?
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.
Yeah. It is the unified CSS selector to control the button length, make sure it does not exceed 16 ch. So will not need to pass down since it is unified among all pickers,
9813e86
to
904aca1
Compare
onClick: () => void; | ||
} | ||
|
||
export const DataSourceMenuPopoverButton: React.FC<DataSourceMenuPopoverButtonProps> = ({ |
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.
nit: can we change the file name as well?
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.
yes
… code Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
35b62b4
to
57cb171
Compare
Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
57cb171
to
91dbbac
Compare
… code (#6559) * Extract the button component for datasource picker to avoid duplicate code Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> * Modify the name of popover button Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> --------- Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> (cherry picked from commit b9ac31e) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
… code (#6559) (#6645) * Extract the button component for datasource picker to avoid duplicate code Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> * Modify the name of popover button Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> --------- Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> (cherry picked from commit b9ac31e) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
… code (opensearch-project#6559) * Extract the button component for datasource picker to avoid duplicate code Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> * Modify the name of popover button Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> --------- Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>
Description
Extract the button component for datasource picker to avoid duplicate code
Screenshot
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration