-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Comments
Another problem with jQuery 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 |
Really? I think
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.
Update: one more thing, the |
Also call TOC here, go or no go. |
TBH, I don't have enough front-end development experience to judge this. In response, I abstain and respect the opinions of other TOC. |
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 |
Most people just copy & paste the existing code in project. They do not care what are the details behind it.
Your You can see there are:
Maybe more.
When? |
Yes, tailwind by default assumes it is the only style system in use and in such a happy world, there is no need for |
So , do not make the Even with the |
BTW I'm tired of having to discuss this |
I have said |
OK, let's focus on this problem. Could you agree with:
|
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 |
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 The divergence is:
There is nothing with the Above is just my opinion, and I am open for TOC's final decision. |
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: |
I guess the question is do we have any other things that would have this proposed 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 I think fomantic's Of course I guess we could actually prefix all of those other In fact if we can just drop all of our modifications from base fomantic, and begin moving code on to So:
What's motivated you into looking in to this? Why now? Why this particular weirdness? |
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 |
Answer to:
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 :
I have realized at that time:
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
|
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 (It would of course be better if we could get all fomantic classes to have a prefix too - e.g. 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: This problem with 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 But let's be honest about what's happening to our styling - it's being turned into tailwind - so let's just do that. |
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:
|
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. |
Okay, I'm also somewhat an outsider when it comes to UI development. Current Behavior
Expected Behavior
My opinionFrom what I've seen now of tailwind, it seems to fix most of these problems, if we refactor everything.
I also don't think overloading classes used for other things already is a good idea.
Regarding how to Other refactors we also need to do at some point
How to manage all these refactoringsIf we just do these things without a "roadmap", as you said, @wxiaoguang, things will fall apart or be left half-finished.
Every other way will likely end in chaos:
|
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 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 So, I think it's better to start cleaning up the hiding-method problem as the first step. |
I'm just looking at rewriting helpers.less to use
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. |
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. |
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>
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>
The final step (all in one) Refactor hiding-methods, remove jQuery show/hide, remove .hide class, remove inline style=display:none #22950 |
…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
The Problem
There are at least 5 different "hiding" methods in code:
.hidden
class[hidden]
attribute.hide
classstyle="display: none"
hide()
(show/toggle)Some of these mechanisms just conflict:
[hidden]
is the weakest one, it will be affected bydisplay: xxx
display: none !important
to overwrite other style'sdisplay: block
.hidden
has other definitions for different selectorshide()
may not work well with somedisplay: none !important
The Solution
.gt-hidden { display: none !important; }
!important
is necessary because there are alwaysblock
orflex
elements need to be hiddengt-
#22879hide()
and related functions to use our.gt-hidden
.hide
class, remove inline style=display:none #22950.gt-hidden
to hideFAQ
Why the
gu-
prefix? (Or something else likegt-
, etc...)The text was updated successfully, but these errors were encountered: