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

feat: add basic-auth UI Form #1718

Merged
merged 14 commits into from
Apr 9, 2021
Merged

feat: add basic-auth UI Form #1718

merged 14 commits into from
Apr 9, 2021

Conversation

LiteSun
Copy link
Member

@LiteSun LiteSun commented Apr 8, 2021

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What changes will this PR take into?
add basic-auth UI Form.

image

Checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@LiteSun LiteSun marked this pull request as ready for review April 8, 2021 10:23
@codecov-io
Copy link

codecov-io commented Apr 8, 2021

Codecov Report

Merging #1718 (0755b95) into master (2c158a1) will decrease coverage by 19.82%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1718       +/-   ##
===========================================
- Coverage   72.08%   52.25%   -19.83%     
===========================================
  Files         130       38       -92     
  Lines        5649     2660     -2989     
  Branches      648        0      -648     
===========================================
- Hits         4072     1390     -2682     
+ Misses       1333     1082      -251     
+ Partials      244      188       -56     
Flag Coverage Δ
backend-e2e-test ?
backend-e2e-test-ginkgo ?
backend-unit-test 52.25% <ø> (-0.04%) ⬇️
frontend-e2e-test ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/internal/utils/version.go 0.00% <0.00%> (-100.00%) ⬇️
api/internal/filter/request_id.go 0.00% <0.00%> (-100.00%) ⬇️
api/internal/core/entity/entity.go 0.00% <0.00%> (-100.00%) ⬇️
api/internal/core/store/storehub.go 0.00% <0.00%> (-71.03%) ⬇️
api/internal/filter/cors.go 0.00% <0.00%> (-66.67%) ⬇️
api/internal/filter/schema.go 0.00% <0.00%> (-55.47%) ⬇️
api/internal/utils/consts/api_error.go 0.00% <0.00%> (-50.00%) ⬇️
api/internal/handler/data_loader/route_import.go 27.41% <0.00%> (-37.50%) ⬇️
api/internal/handler/handler.go 42.59% <0.00%> (-35.19%) ⬇️
api/internal/handler/schema/schema.go 66.66% <0.00%> (-33.34%) ⬇️
... and 115 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c158a1...0755b95. Read the comment docs.

@juzhiyuan
Copy link
Member

please update the codes styles.

@juzhiyuan
Copy link
Member

Good to see i18n

@moonming
Copy link
Member

moonming commented Apr 9, 2021

Is there a preview page?

@LiteSun
Copy link
Member Author

LiteSun commented Apr 9, 2021

Is there a preview page?

Currently, each pr does not have a preview address, it needs to be pulled locally to preview.

@liuxiran
Copy link
Contributor

liuxiran commented Apr 9, 2021

Is there a preview page?

Currently, each pr does not have a preview address, it needs to be pulled locally to preview.

may be provide a video also a good choice, which can explain the changes on the UE more clearly

Copy link
Member

@imjoey imjoey left a comment

Choose a reason for hiding this comment

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

/lgtm

Noted here that the empty list item reflects null instead of `` should be eliminated.

@LiteSun
Copy link
Member Author

LiteSun commented Apr 9, 2021

/lgtm

Noted here that the empty list item reflects null instead of `` should be eliminated.

Thanks for the note, will keep this in mind when dealing with plugins that have list item.

"pageTwoActived": ".ant-pagination-item-2.ant-pagination-item-active",
"selectDropdown": ".ant-select-dropdown",
"codeMirrorMode": "[data-cy='code-mirror-mode']",
"selectJSON":".ant-select-dropdown [label=JSON]"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"selectJSON":".ant-select-dropdown [label=JSON]"
"selectJSON": ".ant-select-dropdown [label=JSON]"

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, will be adjusted in another PR.


export const PLUGIN_UI_LIST = ['basic-auth',];

export const PluginForm: React.FC<Props> = ({ name, renderForm, form }) => {
Copy link
Member

Choose a reason for hiding this comment

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

I started learning .tsx today so I still don't know much. But maybe const line should have ordered arguments ?

Suggested change
export const PluginForm: React.FC<Props> = ({ name, renderForm, form }) => {
export const PluginForm: React.FC<Props> = ({ form, name, renderForm }) => {

Copy link
Member Author

Choose a reason for hiding this comment

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

We are passing in objects, the order of the arguments does not affect the function's functionality IMO, any reference about this rule?


import BasicAuth from './basic-auth'

type Props = {
Copy link
Member

Choose a reason for hiding this comment

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

Does this order work for .tsx?

Suggested change
type Props = {
type Props = {
form: FormInstance,
name: string,
renderForm: boolean
}

]}
/>
<CodeMirror
{Boolean(codeMirrorMode === codeMirrorModeList.UIForm) && <PluginForm name={name} form={UIForm} renderForm={!(pluginType === 'auth' && schemaType !== 'consumer')} />}
Copy link
Member

Choose a reason for hiding this comment

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

Should this be form, name, renderForm ?

Suggested change
{Boolean(codeMirrorMode === codeMirrorModeList.UIForm) && <PluginForm name={name} form={UIForm} renderForm={!(pluginType === 'auth' && schemaType !== 'consumer')} />}
{Boolean(codeMirrorMode === codeMirrorModeList.UIForm) && <PluginForm form={UIForm} name={name} renderForm={!(pluginType === 'auth' && schemaType !== 'consumer')} />}

const { data: yamlData, error } = yaml2json(ref.current.editor.getValue(), true);
if (error) {
notification.error({
message: 'Invalid Yaml data',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
message: 'Invalid Yaml data',
message: 'Invalid YAML data',

@LiteSun LiteSun requested review from nic-chen and Yiyiyimu April 9, 2021 04:26
@LiteSun LiteSun merged commit 82ab89e into apache:master Apr 9, 2021
@LiteSun LiteSun deleted the basic-auth-ui branch April 9, 2021 04:31
@juzhiyuan
Copy link
Member

@jbampton Hi, I just noticed your suggestions :) Could you please file a new PR? due to this change will trigger CI many times 🤣

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.

9 participants