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

UX: Rewrite param-input using FormKit #307

Merged
merged 8 commits into from
Aug 20, 2024
Merged

Conversation

Lhcfl
Copy link
Contributor

@Lhcfl Lhcfl commented Aug 14, 2024

What does this PR do?

This PR refactors param-input to use FormKit. FormKit is a structured form tool in the core. After the rewrite, we will be able to gradually abandon custom validators, get semantic parameter error prompts, etc.

meta link: https://meta.discourse.org/t/wishlist-param-dropdown-for-data-explorer-query/253883/28?u=lhc_fl

Screenshot

image

Lhcfl added 2 commits August 14, 2024 21:15
What does this PR do?
=====================

This PR refactors param-input to use FormKit. FormKit is a structured
form tool in the core. After the rewrite, we will be able to gradually
abandon custom validators, get semantic parameter error prompts, etc.

meta link: https://meta.discourse.org/t/wishlist-param-dropdown-for-data-explorer-query/253883/28?u=lhc_fl
@@ -24,9 +24,17 @@ export default class Query extends RestModel {
return getURL(`/admin/plugins/explorer/queries/${this.id}.json?export=1`);
}

@computed("param_info")
@computed("param_info", "updateing")
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

@nattsw
Copy link
Contributor

nattsw commented Aug 15, 2024

Screenshot 2024-08-15 at 1 58 24 PM

In the screenshot, topic_id and string_list are empty, but one of them is a proper error and one is not. Is this intended?

@nattsw
Copy link
Contributor

nattsw commented Aug 15, 2024

There's a parse error now when group_id is 1.

Screenshot 2024-08-15 at 6 10 29 PM

@Lhcfl
Copy link
Contributor Author

Lhcfl commented Aug 15, 2024

Is this a pre-existing bug? It seems to be a backend behavior, I have not changed this part.

upd: I can reproduce it in main branch, I think we should fix this in another PR

should fix by #308

Lhcfl added a commit that referenced this pull request Aug 15, 2024
We used `with_deleted` incorrectly in the code. This method does not exist
on `User`, `Badge`, and `Group`. This will cause an error when entering a
numeric id in these three parameter input, forcing the user to enter the
name/username of these inputs.

See #307 (comment)
Lhcfl added a commit that referenced this pull request Aug 15, 2024
We used `with_deleted` incorrectly in the code. This method does not exist
on `User`, `Badge`, and `Group`. This will cause an error when entering a
numeric id in these three parameter input, forcing the user to enter the
name/username of these inputs.

See #307 (comment)
@Lhcfl Lhcfl marked this pull request as ready for review August 16, 2024 10:22
Comment on lines +305 to +325
<Form
@data={{this.data}}
@onRegisterApi={{this.onRegisterApi}}
@onSubmit={{this.onSubmit}}
class="params-form"
as |form|
>
{{#each this.paramInfo as |info|}}
<div class="param">
<form.Field
@name={{info.identifier}}
@title={{info.identifier}}
@validation={{info.validation}}
@validate={{info.validate}}
as |field|
>
<info.component @field={{field}} @info={{info}} />
</form.Field>
</div>
{{/each}}
</Form>
Copy link
Contributor

Choose a reason for hiding this comment

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

Im impressed, you have greatly managed to integrate form kit into something which is not "standard" at all 👏

Copy link
Contributor

@nattsw nattsw left a comment

Choose a reason for hiding this comment

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

😽 wonderful work here.

@Lhcfl Lhcfl merged commit 5080ce9 into main Aug 20, 2024
5 checks passed
@Lhcfl Lhcfl deleted the ux-param-input-formkit-rewrite branch August 20, 2024 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants