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

replace jquery-minicolors with coloris (#30055) #30165

Closed

Conversation

GiteaBot
Copy link
Contributor

Backport #30055 by @silverwind

Get rid of one more jQuery dependant and have a nicer color picker as well.

Now there is only a single global color picker init because that is all that's necessary because the elements are present on the page when the init code runs. The init is slightly weird because the module only takes a selector instead of DOM elements directly.

The label modals now also perform form validation because previously it was possible to trigger a 500 error Color cannot be empty. by clearing out the color value on labels.

Screenshot 2024-03-25 at 00 21 05 Screenshot 2024-03-25 at 00 20 48

Get rid of one more jQuery dependant and have a nicer color picker as
well.

Now there is only a single global color picker init because that is all
that's necessary because the elements are present on the page when the
init code runs. The init is slightly weird because the module only takes
a selector instead of DOM elements directly.

The label modals now also perform form validation because previously it
was possible to trigger a 500 error `Color cannot be empty.` by clearing
out the color value on labels.

<img width="867" alt="Screenshot 2024-03-25 at 00 21 05"
src="https://github.com/go-gitea/gitea/assets/115237/71215c39-abb1-4881-b5c1-9954b4a89adb">
<img width="860" alt="Screenshot 2024-03-25 at 00 20 48"
src="https://github.com/go-gitea/gitea/assets/115237/a12cb68f-c38b-4433-ba05-53bbb4b1023e">
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 29, 2024
@GiteaBot GiteaBot added this to the 1.22.0 milestone Mar 29, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 29, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 29, 2024
@lunny
Copy link
Member

lunny commented Mar 29, 2024

Is this backport necessary? Looks like it's not a bugfix.

@lunny lunny modified the milestones: 1.22.0, 1.23.0 Mar 29, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 29, 2024

I think it needs a clear roadmap about how to refactor. I ever wrote something in https://docs.gitea.com/contributing/guidelines-refactoring

Non-bug refactoring is preferred to be done at the beginning of a milestone, it would be easier to find problems before the release.

What I meant for at the beginning of a milestone is:

  1. It should start at next milestone's beginning when the last release is overall stable (eg: after RC or after the first 1.22.1 or 1.22.2 with bug fixes)
  2. It should start before the RC period, to avoid introduce unnecessary regressions.

However, since there are really a lot of frontend refactorings for 1.22 (especially when it was going into freezing time), so I think this time it is still better to merge some PRs back to 1.22, to make the future bug fixes easier to backport.

  • Then clearly define 1.22-rc1 as a "stable" RC, don't backport any unnecessary PRs at that time.

@lunny
Copy link
Member

lunny commented Mar 29, 2024

I think yes. We can backport some necessary to fix bugs easier but that needs case by case. I will not block this one if you think it's necessary.

@silverwind
Copy link
Member

Can skip this one.

@silverwind silverwind closed this Mar 29, 2024
@GiteaBot GiteaBot removed this from the 1.22.0 milestone Mar 29, 2024
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jun 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/dependencies modifies/internal modifies/js modifies/templates This PR modifies the template files 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.

4 participants