Skip to content

[6.0] Replace dialog GIF loader with Custom Element#44232

Closed
C-Lodder wants to merge 3 commits intojoomla:6.0-devfrom
C-Lodder:loader-icon
Closed

[6.0] Replace dialog GIF loader with Custom Element#44232
C-Lodder wants to merge 3 commits intojoomla:6.0-devfrom
C-Lodder:loader-icon

Conversation

@C-Lodder
Copy link
Member

@C-Lodder C-Lodder commented Oct 11, 2024

Summary of Changes

This PR replaces the GIF loading icon used within dialogs, with the Custom Element.

Testing Instructions

Note: This cannot be tested with Patch Tester!

  1. Go to edit any article.
  2. Click "Versions" in the toolbar.
  3. Before the list of versions are loaded, you should see a loading icon for a split second.

Actual result BEFORE applying this Pull Request

image

Expected result AFTER applying this Pull Request

image

cc'ing @Fedik @dgrammatiko

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.2-dev labels Oct 11, 2024
@dgrammatiko
Copy link
Contributor

You could also use import maps here but this is also fine for me

@Fedik
Copy link
Member

Fedik commented Oct 11, 2024

It was made with pure CSS with purpose to allow custom loaders.
Your approach adding a hard dependency. I do not like it, sorry.

@C-Lodder
Copy link
Member Author

C-Lodder commented Oct 11, 2024

@Fedik I changed the Custom Element to use SVG over CSS, cause resizing the CSS one was an absolute nightmare.
Using custom loaders is still possible by overriding the JS file...isn't it?

Either way, this simply implements the loader that's used everywhere else in Joomla, rather than using the outdated GIF, that doesn't look great in dark mode.

@ghost
Copy link

ghost commented Oct 12, 2024

I have tested this item ✅ successfully on 47d29db


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44232.

@richard67
Copy link
Member

@C-Lodder Could you check the two linter errors reported here? https://ci.joomla.org/joomla/joomla-cms/79575/1/20

/drone/src/build/media_source/system/js/joomla-dialog.w-c.es6.js
  319:3  error  Expected 'this' to be used by class method 'renderLoader'  class-methods-use-this
  325:1  error  Trailing spaces not allowed                                no-trailing-spaces

Thanks in advance.

@Fedik
Copy link
Member

Fedik commented Oct 13, 2024

@C-Lodder I agree that the gif on black backround looks ugly,
However, the loader hardcoded in to dialog I do not like even more 😃

Need to find a better solution.
Until then, can just remove the loader CSS, no one will ever notice 😃
Or just make 2 rotating dots with :before, :after animation (there many examples at loading.io/css/, cssload.net etc).

@laoneo
Copy link
Member

laoneo commented Oct 14, 2024

Can you rebase this one to the 5.3-dev branch?

@C-Lodder C-Lodder changed the base branch from 5.2-dev to 5.3-dev October 15, 2024 07:24
@C-Lodder C-Lodder changed the title [5.2] Replace dialog GIF loader with Custom Element [5.3] Replace dialog GIF loader with Custom Element Oct 15, 2024
@C-Lodder
Copy link
Member Author

C-Lodder commented Oct 15, 2024

@laoneo done.

@Fedik Whether the loader is instantiated via CSS class or Javascript, both are couple to something. With CSS, it's coupled to the template. With JS, it coupled to the dialog web component. Both can be overridden.
I can't use the old Joomla CSS loader because it wasn't easily possible to change the size without destroying the layout.
It's also unecessarily coupled to core, so I can't see how a loading icon makes much difference.

Either way, happy to close if anyone would like to provide an alternative, or keep the current icon.

@laoneo laoneo removed the PR-5.2-dev label Oct 15, 2024
@Fedik
Copy link
Member

Fedik commented Oct 15, 2024

core is a different story (it requires only because of Text and HTML sanitisation), but I also was not very happy about that.

it wasn't easily possible to change the size without destroying the layout

I have a hint: width: 30px; height: 30px; background-position:center; 😉

Either way, happy to close if anyone would like to provide an alternative, or keep the current icon.

Hold on for now, I will look what else can be, when will get some time.

@HLeithner HLeithner changed the base branch from 5.3-dev to 6.0-dev March 4, 2025 17:20
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 6.0-dev.

@HLeithner HLeithner changed the title [5.3] Replace dialog GIF loader with Custom Element [6.0] Replace dialog GIF loader with Custom Element Mar 4, 2025
@rdeutz rdeutz removed the PR-5.3-dev label Mar 5, 2025
@exlemor
Copy link

exlemor commented Mar 8, 2025

@Fedik - have you had a chance to think of a new/better solution/option?

@Fedik
Copy link
Member

Fedik commented Mar 8, 2025

No, I forgot 😄
I will check

@Fedik
Copy link
Member

Fedik commented Mar 8, 2025

There is #45097

@exlemor
Copy link

exlemor commented Mar 8, 2025

Thanks @Fedik, I will go test it now...

@Fedik
Copy link
Member

Fedik commented Mar 11, 2025

Okay, that PR was merged, I close this one. Thanks.

@Fedik Fedik closed this Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature NPM Resource Changed This Pull Request can't be tested by Patchtester PR-6.0-dev

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants