Skip to content
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

Fix create source form display and label consistencies #1668

Merged
merged 2 commits into from
Jul 19, 2022

Conversation

clementchong
Copy link
Contributor

Hello

This is for #1292

Please help review. Screenshots after fix are attached in above issue page. Also remove the condition at line 484 to display the form when provider FILE is selected.

@clementchong
Copy link
Contributor Author

local npm run test:ci passed. Do I need submit PR again?

image

@Cuiyansong
Copy link
Contributor

Cuiyansong commented Jul 19, 2022

image

For the "node ci action" failed, you should be add prettier.js plugin for your IDE and format the SourceDetailPage/index.tsx file, then add a new commit to fix format error.

@clementchong
Copy link
Contributor Author

image

For the "node ci action" failed, you should be add prettier.js plugin for your IDE and format the SourceDetailPage/index.tsx file, then add a new commit to fix format error.

Thank you for guidance, this is done and now tests passed. Please help review.

@@ -481,21 +481,20 @@ export function SourceDetailPage() {
</Form.Item>
{dataProviderConfigTemplateLoading && <LoadingOutlined />}

{(providerType !== 'FILE' || formType === CommonFormTypes.Edit) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyp000119 what is provider type file and format type edit, is it necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fyi, I removed it as I see the condition is wrong. On demo site, if I start create a new FILE source, the form does not appear due to this condition. Not sure the reason for having it in the existing code.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyp000119 什么是provider type file和format type edit,有必要吗?

It can be modified like this, no problem

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏿 Good

@clementchong
Copy link
Contributor Author

This display issue was previously raised #1177

However, issue still there on demo site.

@SoarBlueSky SoarBlueSky merged commit 6c73186 into running-elephant:dev Jul 19, 2022
@clementchong clementchong deleted the hotfix_1292 branch July 19, 2022 09:31
@clementchong
Copy link
Contributor Author

Thank you!

@scottsut
Copy link
Contributor

Thanks for the contribution!

@@ -527,7 +526,7 @@ const Content = styled.div`
}

.detailForm {
max-width: ${SPACE_TIMES(240)};
max-width: ${SPACE_TIMES(400)};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change it to 400x?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current JDBC screen has the field labels truncated. See the last few fields below.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current JDBC screen has the field labels truncated. See the last few fields below.

Roger that 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants