-
Notifications
You must be signed in to change notification settings - Fork 920
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
[Workspace] Refactor data source association panel #8383
[Workspace] Refactor data source association panel #8383
Conversation
Signed-off-by: Kapian1234 <wanjinch@amazon.com>
@@ -109,7 +117,7 @@ export const WorkspaceCreatorForm = (props: WorkspaceCreatorFormProps) => { | |||
<EuiSpacer size="m" /> | |||
{/* SelectDataSourcePanel is only visible for dashboard admin and when data source is enabled*/} | |||
{isDashboardAdmin && isDataSourceEnabled && ( | |||
<> |
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 keep this fragment?
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.
ok
)} | ||
<EuiSpacer size="s" /> |
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.
I think these spacer should be inside fragment, then it won't take extra spaces when data source not enabled.
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.
Maybe these spacer could be deleted since we have padding in the panel now
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.
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.
oh, I get it. thank you
Signed-off-by: Kapian1234 <wanjinch@amazon.com>
Signed-off-by: Kapian1234 <wanjinch@amazon.com>
{i18n.translate('workspace.creator.form.associateDataSourceTitle', { | ||
defaultMessage: 'Associate data sources', | ||
<EuiPanel> | ||
<EuiTitle |
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 EuiText + h2
as panel title like this to align with the guidance.
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.
ok
Signed-off-by: Kapian1234 <wanjinch@amazon.com>
showDataSourceManagement={true} | ||
/> | ||
</EuiText> | ||
<SelectDataSourcePanel |
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 more spacing between description and SelectDataSourcePanel?
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.
In SelectDataSourcePanel, there's already a <EuiSpacer size="m" />
, and it would display only when the table exists
Signed-off-by: Kapian1234 <wanjinch@amazon.com>
src/plugins/workspace/public/components/workspace_form/workspace_permission_setting_input.tsx
Outdated
Show resolved
Hide resolved
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.
@Kapian1234 I think this file should not be included in this PR. Can we remove it?
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.
removed
Signed-off-by: Kapian1234 <wanjinch@amazon.com>
Signed-off-by: Kapian1234 <wanjinch@amazon.com>
* refactor association panel Signed-off-by: Kapian1234 <wanjinch@amazon.com> * Changeset file for PR #8383 created/updated * resolve some issues Signed-off-by: Kapian1234 <wanjinch@amazon.com> * update title style Signed-off-by: Kapian1234 <wanjinch@amazon.com> * update title style Signed-off-by: Kapian1234 <wanjinch@amazon.com> * update placeholder Signed-off-by: Kapian1234 <wanjinch@amazon.com> * update placeholder Signed-off-by: Kapian1234 <wanjinch@amazon.com> * / Signed-off-by: Kapian1234 <wanjinch@amazon.com> --------- Signed-off-by: Kapian1234 <wanjinch@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 83d5fba) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* refactor association panel * Changeset file for PR #8383 created/updated * resolve some issues * update title style * update title style * update placeholder * update placeholder * / --------- (cherry picked from commit 83d5fba) Signed-off-by: Kapian1234 <wanjinch@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
…t#8383) (opensearch-project#8419) * refactor association panel * Changeset file for PR opensearch-project#8383 created/updated * resolve some issues * update title style * update title style * update placeholder * update placeholder * / --------- (cherry picked from commit 83d5fba) Signed-off-by: Kapian1234 <wanjinch@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
Refactor data source association panel
Issues Resolved
Screenshot
before
after
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration