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

Remove fomantic transition module #26469

Merged
merged 16 commits into from
Aug 16, 2023
Merged

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Aug 12, 2023

Removes all dropdown and dimmer animations. Works everywhere as far as I can tell, but need to give this thorough testing. Removes around 70kb JS/CSS.

Note, I'm not 100% sure regarding the various callbacks, those will need more investigation, but it appears to work nonetheless.

Fixes: #15709

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 12, 2023
@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 12, 2023
@silverwind silverwind added the type/enhancement An improvement of existing functionality label Aug 12, 2023
@silverwind
Copy link
Member Author

silverwind commented Aug 12, 2023

Has bugs with menu hiding in the navbar because of interference from our own styles, wip.

@silverwind silverwind marked this pull request as draft August 12, 2023 14:48
@silverwind silverwind marked this pull request as ready for review August 12, 2023 14:52
@silverwind
Copy link
Member Author

silverwind commented Aug 12, 2023

Hiding bug fixed. Fomantic apparently never removes the transition class after transition, so I replicated that and that makes the hiding CSS in nav work again.

web_src/js/modules/fomantic.js Outdated Show resolved Hide resolved
web_src/js/modules/fomantic.js Outdated Show resolved Hide resolved
@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 Aug 12, 2023
@wxiaoguang
Copy link
Contributor

It breaks the modal dialogs.

@silverwind silverwind marked this pull request as draft August 14, 2023 12:05
@silverwind
Copy link
Member Author

I see what you mean, the position is incorrect, but modal works otherwise. This was not an issue in an earlier iteration of this, checking...

Screenshot 2023-08-14 at 15 01 49

@silverwind
Copy link
Member Author

Problem is the display: flex !important; inline style is missing, but interestingly, the module does not contain flex in the source.

@wxiaoguang
Copy link
Contributor

IIRC Fomantic use some "visibility" tricks for showing/hiding some elements (with animation), but I can't tell the details at the moment .... too complex 😄

@silverwind silverwind marked this pull request as ready for review August 14, 2023 13:26
@silverwind
Copy link
Member Author

Modal should be fixed. This replacement function is likely not 100% equivalent to the removed module, but it appears to work well enough.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 15, 2023

TBH, I am not a fan of removing all animations.

According to modern web sites:

  1. For dropdowns, it doesn't need to use animation in most cases
  2. For fly-in and modal dialogs, in most cases, they use animations, I have checked Apple, Google, including GitHub's fly-in panels

So I do not think it's right to remove the animations totally.

@silverwind
Copy link
Member Author

silverwind commented Aug 15, 2023

I like them nowhere and selectively allowing some would be a much more deeper dive into the internals of Fomantic that I'm not interested in doing.

Is it really so bad to not have the modal backdrop fade-in? Maybe if you really want, you could try adding a CSS transition to it, but I suppose without support in the function, it won't transition.

@delvh
Copy link
Member

delvh commented Aug 15, 2023

Have tested it.
Yes, something feels a little off when there's no delay for a modal.
But that sounds like a relatively easy fix where we simply add a small ease-in-out transition ourselves on all modals (.modal).
I don't think we need to keep the Fomantic module for that.

@silverwind
Copy link
Member Author

silverwind commented Aug 15, 2023

Checked how GitHub does it, they have a 300ms fade-in (assume it's just opacity 0 to 1) animation on the modal content, but not the backdrop (which shows instantly). Hide is instant on both backdrop and modal, so there is no fade-out.

@silverwind
Copy link
Member Author

silverwind commented Aug 15, 2023

Modal now fades in. I also added a new CSS variable --color-overlay-backdrop for the backdrop color and very slightly reduced it's opacity over its default rgba(0,0,0,.85). I think it will be useful for themes.

Screenshot 2023-08-15 at 15 21 30 Screenshot 2023-08-15 at 15 21 41

@silverwind
Copy link
Member Author

silverwind commented Aug 15, 2023

One more tweak done: added box shadows to dropdowns so they stand out more against the background:

Screenshot 2023-08-16 at 00 26 25 Screenshot 2023-08-16 at 00 26 14 Screenshot 2023-08-16 at 00 36 24 Screenshot 2023-08-16 at 00 36 12

(subpixel misalignment in second screenshot is a fomantic bug)
also, fixed an issue with the border color changing unexpectedly on focus change.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Haven't tested, while it might be worth to try.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 16, 2023
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 16, 2023
@silverwind silverwind enabled auto-merge (squash) August 16, 2023 22:03
@silverwind silverwind merged commit 376c0e2 into go-gitea:main Aug 16, 2023
23 checks passed
@GiteaBot GiteaBot added this to the 1.21.0 milestone Aug 16, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 16, 2023
@silverwind silverwind deleted the rmtransition branch August 16, 2023 23:12
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 17, 2023
* upstream/main:
  Sync repo's IsEmpty status correctly (go-gitea#26517)
  [skip ci] Updated translations via Crowdin
  Remove fomantic transition module (go-gitea#26469)
  Explain SearchOptions and fix ToSearchOptions (go-gitea#26542)
  Update go dependencies (go-gitea#26534)
  Differentiate better between user settings and admin settings (go-gitea#26538)
  Add missing triggers to update issue indexer (go-gitea#26539)
  Improve deadline icon location in milestone list page (go-gitea#26532)
  Use unique class for breadcrumb divider (go-gitea#26524)
  Fix typo of RunerOwnerID (go-gitea#26508)
  Improve clickable area in repo action view page (go-gitea#26115)
  Fix dark theme highlight for "NameNamespace" (go-gitea#26519)
  Remove duplicate CSS import for chroma/base.css (go-gitea#26523)
  Fix project filter bugs (go-gitea#26490)
  Fix display problems of members and teams unit (go-gitea#26363)
  Use `hidden` over `clip` for text truncation (go-gitea#26520)
  Add API route to list org secrets (go-gitea#26485)
  Set "type=button" for editor's toolbar buttons (go-gitea#26510)
  Apply to become a maintainer (go-gitea#26514)
  Detect ogg mime-type as audio or video (go-gitea#26494)
silverwind added a commit that referenced this pull request Aug 21, 2023
Add `box-shadow` replacement to the `floating` dropdown variant as well,
which was missed in #26469. The
Fomantic style has `!important`, so this has to have too. Also made a
tiny adjustment to shadow color on dark theme.

<img width="305" alt="Screenshot 2023-08-18 at 16 40 34"
src="https://github.com/go-gitea/gitea/assets/115237/a0aac9cb-6393-4d69-b0b3-00eaac5ccf9f">
<img width="202" alt="Screenshot 2023-08-18 at 16 40 22"
src="https://github.com/go-gitea/gitea/assets/115237/0a5fa3aa-7452-4dbd-86ed-ccbc1c872ebb">

Co-authored-by: Giteabot <teabot@gitea.io>
lunny pushed a commit that referenced this pull request Aug 28, 2023
Remove transition related code because the transition module has been
removed by #26469
wxiaoguang added a commit that referenced this pull request Aug 28, 2023
1. Fine tune the CSS styles, and add more examples
2. Add necessary "dimmer" animation for modal dialogs, otherwise the UI
seems flicking (follow #26469)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] option to disable animations
4 participants