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

New webhook page supports quick switching type #26941

Closed
wants to merge 21 commits into from

Conversation

kerwin612
Copy link
Member

New webhook page supports quick switching type
d0ce1cddef455604d04c89408b0224f

Sometimes user select the wrong type when creating a new webhook. User need to return to the list page and click the Add button again to select it.
This PR is in the add page, user can click the type-icon in the right-top-corner to quickly switch different types of webhooks.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 6, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 6, 2023
@lunny lunny added the type/enhancement An improvement of existing functionality label Sep 7, 2023
@lunny lunny added this to the 1.21.0 milestone Sep 7, 2023
@puni9869
Copy link
Member

Please don't do this. :( Try to give a meaningful commit message. Easy for maintainers to review the PR commit by commit.

image

Here is the Guideline.
https://github.com/go-gitea/gitea/blob/main/CONTRIBUTING.md

…w webhook page supports quick switching type
@kerwin612
Copy link
Member Author

Please don't do this. :( Try to give a meaningful commit message. Easy for maintainers to review the PR commit by commit.

image

Here is the Guideline. https://github.com/go-gitea/gitea/blob/main/CONTRIBUTING.md

Sorry, that is a temp log, accidentally submitted up; I've re-edited it now.

@puni9869
Copy link
Member

Please don't do this. :( Try to give a meaningful commit message. Easy for maintainers to review the PR commit by commit.
image
Here is the Guideline. https://github.com/go-gitea/gitea/blob/main/CONTRIBUTING.md

Sorry, that is a temp log, accidentally submitted up; I've re-edited it now.

Thanks @kerwin612 for considering this feedback and making maintainers life easy for future. Appreciated your efforts in this PR.
Kudos 💯

@lunny lunny modified the milestones: 1.21.0, 1.22.0 Sep 21, 2023
@kerwin612 kerwin612 requested a review from a team October 21, 2023 00:13
@kerwin612 kerwin612 requested a review from lunny November 2, 2023 00:15
@silverwind
Copy link
Member

silverwind commented Nov 2, 2023

This has multiple issues. On overview page, it renders a empty button:

image

On view page, it renders the icon partially outside the header (because english font is not as large as chinese):

Screenshot 2023-11-02 at 20 05 09

Also, the UX with that button seems strange. I could not guess this icon is clickable by looking at the UI.

I feel like webhook type should be a <select> style dropdown element in the form itself, and switching it should ideally also preserve any previously entered values for fields that are in both old and new form.

@kerwin612 kerwin612 marked this pull request as draft November 3, 2023 03:30
@kerwin612
Copy link
Member Author

This has multiple issues. On overview page, it renders a empty button:

image On view page, it renders the icon partially outside the header (because english font is not as large as chinese): Screenshot 2023-11-02 at 20 05 09 Also, the UX with that button seems strange. I could not guess this icon is clickable by looking at the UI.

I feel like webhook type should be a <select> style dropdown element in the form itself, and switching it should ideally also preserve any previously entered values for fields that are in both old and new form.

When I submit PR, the style issues you mentioned does not exist. It may be that the modification of master during the period brings compatibility problems. I will re-adapt it. I will also consider the solution that you mentioned to take webhook type as part of form and replace it with <select>.

@silverwind
Copy link
Member

<select> dropdown would be ideal, you can take a hint from user management where depending on the value of the auth source dropdown, the form's fields change:

image

@kerwin612
Copy link
Member Author

<select> dropdown would be ideal, you can take a hint from user management where depending on the value of the auth source dropdown, the form's fields change:

image

I tried what you said and it didn't look good because the webhook type with icon;

@silverwind
Copy link
Member

You wouldn't show a icon in this case, just a dropdown with the type name like in the screenshot.

@kerwin612
Copy link
Member Author

You wouldn't show a icon in this case, just a dropdown with the type name like in the screenshot.

I completely understand; however, what I considered at the beginning was that the entire form was divided into different templates according to different types, and It is not appropriate to put the switch type in each form template, so I thought of using the icon in the upper right corner to switch;
image

@kerwin612
Copy link
Member Author

The current PR UX is as follows:
image
image

@kerwin612
Copy link
Member Author

kerwin612 commented Nov 5, 2023

Another implementation is as follows:
image
image

@silverwind @lunny

@lunny
Copy link
Member

lunny commented Nov 5, 2023

Another implementation is as follows: image image

@silverwind @lunny

I would like the second, some benefits we will gain from the design.

  • It's straight forward
  • It's possible to change the following options according the first selection
  • An search box could be added in webhook type selection box

@github-actions github-actions bot added modifies/translation and removed modifies/templates This PR modifies the template files labels Nov 7, 2023
@kerwin612
Copy link
Member Author

Another implementation is as follows: image image
@silverwind @lunny

I would like the second, some benefits we will gain from the design.

  • It's straight forward
  • It's possible to change the following options according the first selection
  • An search box could be added in webhook type selection box

I merged the second implementation into the current branch.
I don't think there's a need to add a search box to the current (fewer) options.
Teams discuss it? I look forward to hearing from you!

@kerwin612 kerwin612 requested a review from a team November 8, 2023 01:30
@denyskon denyskon removed the request for review from a team January 15, 2024 19:12
@silverwind
Copy link
Member

Maybe I'm wrong, but I think #29114 has now implemented what this PR set out to do, right?

@lunny lunny modified the milestones: 1.22.0, 1.23.0 Mar 29, 2024
@kerwin612
Copy link
Member Author

Maybe I'm wrong, but I think #29114 has now implemented what this PR set out to do, right?

I don't understand very much, I submitted this PR in 23 years, and also made changes according to the audit suggestions, why there has been no administrator response to this PR, and the PR you mentioned was submitted in 24 years, and it can be merged very quickly;

@kerwin612 kerwin612 closed this Apr 2, 2024
@lunny
Copy link
Member

lunny commented Apr 2, 2024

Maybe I'm wrong, but I think #29114 has now implemented what this PR set out to do, right?

I don't understand very much, I submitted this PR in 23 years, and also made changes according to the audit suggestions, why there has been no administrator response to this PR, and the PR you mentioned was submitted in 24 years, and it can be merged very quickly;

Sorry for the slow reviews. Can #29114 satisfy your requirements?

@GiteaBot GiteaBot removed this from the 1.23.0 milestone Apr 2, 2024
@kerwin612 kerwin612 deleted the patch-1 branch June 5, 2024 06:53
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jul 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/translation size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants