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

[Proposal] Refactor frontend hide/hidden mechanism #22847

Closed
wxiaoguang opened this issue Feb 10, 2023 · 26 comments · Fixed by #22950
Closed

[Proposal] Refactor frontend hide/hidden mechanism #22847

wxiaoguang opened this issue Feb 10, 2023 · 26 comments · Fixed by #22950
Labels
topic/ui Change the appearance of the Gitea UI type/proposal The new feature has not been accepted yet but needs to be discussed first. type/refactoring Existing code has been cleaned up. There should be no new functionality.

Comments

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 10, 2023

The Problem

There are at least 5 different "hiding" methods in code:

  • Fomantic-UI's .hidden class
  • Browser and Fomantic-UI's [hidden] attribute
  • Gitea's .hide class
  • Inline style="display: none"
  • jQuery's hide() (show/toggle)

Some of these mechanisms just conflict:

  • [hidden] is the weakest one, it will be affected by display: xxx
  • Sometimes, it should use display: none !important to overwrite other style's display: block
  • Fomantic-UI's .hidden has other definitions for different selectors
  • jQuery's hide() may not work well with some display: none !important

The Solution

FAQ

Why the gu- prefix? (Or something else like gt-, etc...)

  • It means "Gitea UI", it's time to have our own frontend framework and have a global design, instead of patching everything again and again.
  • It is a clear style, won't be polluted by Fomantic UI
  • It makes the refactoring clear, every changed line could be reviewed to check whether something is affected.
  • It makes it easier to be searched from whole code base, since it's a dedicated name.
  • It's widely accepted to add prefixes for frameworks. For example, tailwind also supports prefix: https://tailwindcss.com/docs/configuration#prefix
@wxiaoguang wxiaoguang added type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first. labels Feb 10, 2023
@lunny lunny added type/refactoring Existing code has been cleaned up. There should be no new functionality. and removed type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Feb 10, 2023
@silverwind
Copy link
Member

silverwind commented Feb 10, 2023

Another problem with jQuery hide and show is that like any other inline CSS, it does not work without CSP unsafe-inline, so that is another reason to remove it. Als with the removal enable the eslint rules jquery/no-hide and jquery/no-show to detect cases and forbid them in future code.

I'm not sure about the classname prefix. I'm more of the opinion we should eventually migrate to tailwind or one of it's many "forks", and for this use case, we better match the helper names to those frameworks (thought I imagine we can not get around the !important requirement for the helpers until we remove fomantic-ui completely.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Feb 10, 2023

Another problem with jQuery hide and show is that like any other inline CSS, it does not work without CSP unsafe-inline, so that is another reason to remove it. Als with the removal enable the eslint rules jquery/no-hide and jquery/no-show to detect cases and forbid them in future code.

Really? I think unsafe-inline only forbids the style="xxx" in HTML, but it doesn't forbid changing style by JS, right? Correct me if I am wrong.

I'm not sure about the classname prefix. I'm more of the opinion we should eventually migrate to tailwind or one of it's many "forks", and for this use case, we better match the helper names to those frameworks (thought I imagine we can not get around the !important requirement for the helpers until we remove fomantic-ui completely.

About "eventually migrate to": I am not sure how long does "eventually" mean. If it means years, I wouldn't say that's practicable or feasible. I have explained the refactoring problem in the jQuery-dropping discussion: if there is no plan to guarantee that the refactoring can succeed in short time, people come and go, code using old framework always comes, then it will become a endless whack-a-mole game.

The prefix is a must.

  • Your .hidden from #22845 is different from tailwind or other frameworks. They would conflict in the end.
  • The prefix makes it easier to be globally searched and replaced.

Update: one more thing, the !important seems necessary in Gitea's frontend framework, because there are always some block or flex elements need to be hidden.

@wxiaoguang
Copy link
Contributor Author

Also call TOC here, go or no go.

@lunny @techknowlogick @6543 @zeripath @jolheiser @wolfogre

@wolfogre
Copy link
Member

TBH, I don't have enough front-end development experience to judge this. In response, I abstain and respect the opinions of other TOC.

@silverwind
Copy link
Member

silverwind commented Feb 10, 2023

It seems needlessly complex to me to introduce a gitea-specific class prefix. Some people are accustomed to tailwind classes, so I would make it easy for them and use the original class names (which may conflict with fomantic, but fomantic needs to go sooner or later anyways).

If possible, I'd like us to implement tailwind-compatible classes, but I'm not sure how the tooling can be made to parse the golang templates so their tree-shaking of classes can work. I imagine a special parser will be necessary to extract classes in-use from the go templates. Also, I'm pretty sure we will need the !important suffix on all rules, at least until we get rid of fomantic.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Feb 10, 2023

Some people are accustomed to tailwind classes,

Most people just copy & paste the existing code in project. They do not care what are the details behind it.

so I would make it easy for them and use the original class names (which may conflict with fomantic,

Your .hidden with !important is not really tailwind style. Tailwind's hidden doesn't have the !important, the different behavior may surprise developer much more.

You can see there are:

  1. .ui.checkbox input.hidden { z-index: -1;}
  2. .ui.menu .hidden { visibility: none;}

Maybe more.

but fomantic needs to go sooner or later anyways). .... at least until we get rid of fomantic.

When?

@silverwind
Copy link
Member

silverwind commented Feb 10, 2023

Your .hidden with !important is not really tailwind style. Tailwind's hidden doesn't have the !important, the different behavior may surprise developer much more.

Yes, tailwind by default assumes it is the only style system in use and in such a happy world, there is no need for !important, but as soon as you add another, specificity issues are bound to happen and can only be remedied by !important.

@wxiaoguang
Copy link
Contributor Author

So , do not make the .hidden harder to understand.

Even with the .gu- prefix, after removing the fomantic, you can do a global replace to rename it to anything you like. That's the good side of the prefix.

@silverwind
Copy link
Member

BTW I'm tired of having to discuss this !important topic again and again. It is a necessary evil that can not be removed unless you have a single style system which will not happen until at least fomantic is out.

@wxiaoguang
Copy link
Contributor Author

I have said !important is necessary for this case (many times in this issue). My point is clear: do not make surprise for developers. Make every name clear and unique/dedicated.

@wxiaoguang
Copy link
Contributor Author

OK, let's focus on this problem. Could you agree with:

  • If most people like the .hidden style, you do the refactoring (you choose the time)
  • If most people like the .gu-hidden style, I do the refactoring (I will try to finish it in a few days)
  • If few people like this proposal, let's close it (in 7 days)

@jolheiser
Copy link
Member

Tailwind or not, is there any way to do a gradual transition? And which style do you think would make that hypothetical transition easier?

If parsing the templates is doable, and we can post-process to add !important, maybe that's feasible?

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Feb 10, 2023

Tailwind or not, is there any way to do a gradual transition? And which style do you think would make that hypothetical transition easier?

The transition plan is: removing Fomantic modules one by one, for example, the tooltip, image, ... modules have been removed, customized dropdown has been reverted to the official one, etc. Maybe the checkbox module could be removed next.

But this style discussion is not related to the transition. The hidden class is an essential CSS class, it's the lowest-level design.

The divergence is:

  • silverwind prefers to re-use the .hidden name from Fomantic UI, which is (in my mind):
    • looks like a tailwind class name but it's different from the tailwind's one
    • has been polluted by the Fomantic UI framework in some cases
  • I prefer to use a totally new/dedicated name with prefix: .gu-hidden, then:
    • new developers should get used to it (they could just copy & paste existing code)
    • is a clear style without any pollution, won't cause surprises for developers of tailwind or other frameworks
    • could be replaced to any new name after removing Fomantic

If parsing the templates is doable, and we can post-process to add !important, maybe that's feasible?

There is nothing with the !important indeed, it is necessary for either .hidden or .gu-hidden.


Above is just my opinion, and I am open for TOC's final decision.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Feb 10, 2023

One more thing worth to be mentioned: it's widely accepted to add prefixes for frameworks. For example, tailwind also supports prefix: https://tailwindcss.com/docs/configuration#prefix . If we want to introduce it, the prefix is always the best choice, to avoid unnecessary conflicts: .tw-hidden, .tw-text-left, etc.

@zeripath
Copy link
Contributor

zeripath commented Feb 10, 2023

I guess the question is do we have any other things that would have this proposed gu- prefix?

At present, this would stand out completely separately from the rest of our class names - and I'm not sure of the benefit when we could just use .hide which is essentially your proposed style just lacking the !important.

I think fomantic's .hidden has different definitions for different selectors because the proposed display: none !important; is a bit of blunderbuss and fomantic has its own semantics related to hidden that are not completely about making things absolutely invisible. Thus I think overloading that will be a bad idea.

Of course I guess we could actually prefix all of those other !important class modifiers that we have, e.g. mr-2 becomes gu-mr-2 that would at least make them less likely to conflict with things and make us more likely to just move them into classes - like they should be.

In fact if we can just drop all of our modifications from base fomantic, and begin moving code on to gitea- classes that might, just might, make extracting out fomantic easier.


So:

  • 👎 to overloading .hidden - it will conflict with fomantic. We're already seeing issues where we've broken fomantic's styling and we blame it when it's often due to styling we've imposed on top of it.
  • 〰️ I'm uncertain about the gu- prefix - having only one class with the prefix is janky as hell. If there are others then it makes more sense.
  • 〰️ I think just using .hide might be the answer - leave .hidden for fomantic.

What's motivated you into looking in to this? Why now? Why this particular weirdness?

@lunny
Copy link
Member

lunny commented Feb 11, 2023

I learnt something from introducing #21986 . If we want to do a big refactor, introduce a new method/mechanism is easier than replacing the old method/mechanism. If this is the beginning of the refactor from Fomantic-UI to Tailwind, I agree to have a prefix will be better for the progress of migrating. For the prefix name, we could have more thought than gu.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Feb 11, 2023

Answer to:

What's motivated you into looking in to this? Why now? Why this particular weirdness?

I have been thinking about this problem since my first frontend refactoring plan (the time I started spending time on Gitea), see #17149 (I would suggest people to read it again .... those are all mistakes Gitea had made in history) and related https://docs.gitea.io/en-us/guidelines-frontend/#gitea-specific-guidelines :

CSS styling for classes provided by frameworks should not be overwritten. Always use new class-names with 2-3 feature related keywords to overwrite framework styles.

I have realized at that time:

  • the frontend is becoming more and more messy
  • the messy code causes more and more problems
  • sometimes people complains Fomantic but actually the problem is cauesd by Gitea itself

But there were a lot of obstructions for doing refactoring. Recently there is TOC for Gitea, I'd like to see whether it's possible to push the refactoring and make the codebase more healthy.

For the hiding problem itself:

@zeripath
Copy link
Contributor

zeripath commented Feb 12, 2023

OK, the #22845 farago has absolutely convinced me.

The helpers MUST have a prefix.

I've just looked at tailwind and now it's clear to me what's happening to our styling over time... and it's clear to me that we're going to have more and more conflict with fomantic over time and if we're going to do that, we should do it using a prefix. I guess the intention is that a tailwind website has no separate less/css and everything is just done with classes in the html? Well if we want to do that - it should be easy to just double add classes for fomantic and tailwind with the tailwind classes prefixed with tw- and you can turn on and off styling with fomantic by switching in and out style file loading.

(It would of course be better if we could get all fomantic classes to have a prefix too - e.g. .fo-ui but that would any and all of the downstream themes and stylers, or get the fomantic classes to only fire if there is some root class but I don't think it can be done.)

Therefore, in addition, we should start putting a prefix on all of own document-semantic classes because otherwise it's just too hard to separate them from fomantic's.

One thing that's clear to me is that as people are moving to tailwind one of the things we're going to lose is the ability to use fomantic's documentation as descriptors for prototyping. So if the intention is tailwind we're going to need common examples of what we we're going to use.


I do think it's worth saying again - our overloading of fomantic styling has and is causing a number of problems, for example, at some point we had in our less files: .right { float: right;} (we may still have this), unsurprisingly this breaks lots of fomantic styling which uses/used .right in a more complex and subtle way.

This problem with .hidden above is similar.

Far too often we're not working with fomantic styling - we're working against it by treating it as if its tailwind when it is not - and this is at least partially due to the fact that we've already broken fomantic's styling in many places so when things are broken we're not aware of why and we blame fomantic unfairly.

However, I don't think we can go back to a situation where we can fix our styling to be compatible with plain fomantic anymore - certainly I don't think people want to. The fragility we have isn't going to get any better until we've got the templates not dependent on fomantic - so lets just do that.

Let's just follow the plan in here:

https://dev.carsonbain.com/blog/migrate-project-to-tailwind-css

Markup the templates with non-tailwind helpers having g-h- prefix gt- has been chosen and tailwind classes with a tw- prefix and then get it to a point where we can drop fomantic. Consider using js- class for hooks for javascript or id where appropriate and stick gitea- prefixes on specific components.

But let's be honest about what's happening to our styling - it's being turned into tailwind - so let's just do that.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Feb 12, 2023

I'm afraid we can not just follow everything in https://dev.carsonbain.com/blog/migrate-project-to-tailwind-css. It only works for a team with strong leadership.

TLDR: The migration needs detailed plan and guideline for Gitea.

There are some problems for Gitea:

  1. How to "freeze" old code? Especially when new PR comes? Suggest contributors to rewrite? Maintainers help to rewrite?
  2. I do not feel that Gitea can introduce tailwind with !important, otherwise some Gitea's styles may need doube-important to overwrite the tailwind styles. So it requires that the new code should be clear on Gitea side.
  3. There should be a detailed plan and guideline to guarantee the migration could succeed in predictable time and the work could be pushed to move on regularly, otherwise, the mixed code will become the next DashboardRepoList or RepoBranchTagDropdown -- which mix Go Template/Fomantic UI/Vue3/jQuery together. Maybe nobody wants to touch them (maybe except me because I had plan to refactor them ....)

@zeripath
Copy link
Contributor

That's what I'm trying to suggest here.

It's clear that we're migrating to a tailwind situation already so let's stop pretending.


You want to help? Come back and let's just do that. A cabal of three people is all it needs.

@delvh
Copy link
Member

delvh commented Feb 12, 2023

Okay, I'm also somewhat an outsider when it comes to UI development.
The following is purely based on my previous experience here, and what I read in this thread so far:

Current Behavior

  • CSS Selectors are endlessly nested, which makes it fairly unpredictable which classes will influence a given element
  • There are way too many ways to achieve something, and they all contradict each other in some way
  • There are way too many !importants. How is anyone supposed to know what will work, and what won't?

Expected Behavior

  • The points above have been addressed

My opinion

From what I've seen now of tailwind, it seems to fix most of these problems, if we refactor everything.
I agree with you, @wxiaoguang, this will only work if we set a strict roadmap for how to approach that refactoring.
I am already surprised that we managed three partial major refactors for 1.19:

  • less models/services imports inside modules
  • relative instead of absolute links
  • a11y

I also don't think overloading classes used for other things already is a good idea.
Nor do I think it's a good idea to follow point three of the blog linked above (everything !important).
The other two points sound fine (prefix for tailwind classes, and freeze old code).
Yes, if we keep consistent with our class prefixes, this should make a lot of things much easier.
I agree with @zeripath that the best prefix distribution is probably

  • tw- for tailwind classes
  • gitea- for semantic classes (i.e. gitea-diff-box)
  • js- for classes queried in JS

Regarding how to freeze old code: I think that's pretty simple to achieve:
Simply change the corresponding filenames by prepending a legacy- for example.
That way, it's easy to see what is new, and what isn't.

Other refactors we also need to do at some point

  • ctx for most methods as first parameter
  • the remaining models/services imports inside modules
  • a11y
  • migrate logging to slog
  • better documentation
  • remove custom functions with exp/maps / exp/slices once they are not exp anymore.

How to manage all these refactorings

If we just do these things without a "roadmap", as you said, @wxiaoguang, things will fall apart or be left half-finished.
I think there's only one way to ensure all refactorings are completed:
We need a specific "refactoring manager" that is responsible for

  • setting up a roadmap how to complete a given refactoring
  • assigning deadlines to the given roadmap items
  • encouraging people to post PRs to hit the refactoring goal
  • knowing how far each refactoring has progressed

Every other way will likely end in chaos:

  • No coordination: worst case.
  • TOC oversees it: Likely everyone has enough tasks already and no one will feel responsible, so it will probably be forgotten
  • Documented roadmap only: The percentage of people reading through structural documents is marginal, so it won't be done either
  • anything else I forgot?

@wxiaoguang
Copy link
Contributor Author

Hmm, I see. Maybe the tailwind topic is a little off-topic of this proposal.

Let's back to this proposal, and postpone the tailwind topic to another proposal.

TLDR: tailwind's hidden couldn't help Gitea at the moment, Gitea needs its own hidden mechanism.

Details: This proposal's hiding-method problem should be resolved by Gitea itself, but can not be helped by tailwind. The reason is that there are still a lot of jQuery code need to do show/hide, and we need our hidden style to hide some display: block/flex elements.

So, I think it's better to start cleaning up the hiding-method problem as the first step.

@zeripath
Copy link
Contributor

I'm just looking at rewriting helpers.less to use g-h- as a prefix and I've already found one conflict:

.sb

This is used in chroma for its type to represent a string backtick but it has a helper class applied to it by the helpers.less that fortunately doesn't ruin the styling but is a conflict.

@lunny
Copy link
Member

lunny commented Feb 12, 2023

Hmm, I see. Maybe the tailwind topic is a little off-topic of this proposal.

Let's back to this proposal, and postpone the tailwind topic to another proposal.

TLDR: tailwind's hidden couldn't help Gitea at the moment, Gitea needs its own hidden mechanism.

Details: This proposal's hiding-method problem should be resolved by Gitea itself, but can not be helped by tailwind. The reason is that there are still a lot of jQuery code need to do show/hide, and we need our hidden style to hide some display: block/flex elements.

So, I think it's better to start cleaning up the hiding-method problem as the first step.

Would you create a new issue about the frontend refactors? I prefer that we can have a principle like refactoring review/merge first, then we can keep fewer conflicts between refactors and features.

zeripath added a commit to zeripath/gitea that referenced this issue Feb 12, 2023
As discussed in go-gitea#22847 the helpers in helpers.less need to have a
separate prefix as they are causing conflicts with fomantic styles

This will allow us to have the `.gt-hidden { display:none !important; }`
style that is needed to for the reverted PR.

Of note in doing this I have noticed that there was already a conflict
with at least one chroma style which this PR now avoids.

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit that referenced this issue Feb 13, 2023
As discussed in #22847 the helpers in helpers.less need to have a
separate prefix as they are causing conflicts with fomantic styles

This will allow us to have the `.gt-hidden { display:none !important; }`
style that is needed to for the reverted PR.

Of note in doing this I have noticed that there was already a conflict
with at least one chroma style which this PR now avoids.

I've also added in the `gt-hidden` style that matches the tailwind one
and switched the code that needed it to use that.

Signed-off-by: Andrew Thornton <art27@cantab.net>

---------

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@wxiaoguang
Copy link
Contributor Author

About "Would you create a new issue about the frontend refactors?", see Add some guidelines for refactoring #22880

And the first step is ready for review: Extend jQuery with customized show/hide/toggle #22884

@wxiaoguang
Copy link
Contributor Author

The final step (all in one) Refactor hiding-methods, remove jQuery show/hide, remove .hide class, remove inline style=display:none #22950

lunny pushed a commit that referenced this issue Feb 19, 2023
…s, remove inline style=display:none (#22950)

Close #22847

This PR:

* introduce Gitea's own `showElem` and related functions
* remove jQuery show/hide
* remove .hide class
* remove inline style=display:none 

From now on:

do not use:
* "[hidden]" attribute: it's too weak, can not be applied to an element
with "display: flex"
* ".hidden" class: it has been polluted by Fomantic UI in many cases
* inline style="display: none": it's difficult to tweak
* jQuery's show/hide/toggle: it can not show/hide elements with
"display: xxx !important"

only use:
* this ".gt-hidden" class
* showElem/hideElem/toggleElem functions in "utils/dom.js"

cc: @silverwind , this is the all-in-one PR
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
topic/ui Change the appearance of the Gitea UI type/proposal The new feature has not been accepted yet but needs to be discussed first. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
8 participants