Skip to content

Conversation

mahmoudmagdy1-1
Copy link

@mahmoudmagdy1-1 mahmoudmagdy1-1 commented Mar 18, 2025

Pull Request for Issue # .

Summary of Changes

I've implemented the createProgressBar() and removeProgressBar() methods in the Media Manager's image editor. These methods were previously marked with TODO comments but now have implementations that handle creating and removing the progress indicator element during image edits.

I've implemented two of the three TODO methods createProgressBar and removeProgressBar and I've used the sass class that's used for loading the progress bar in the media manager (media-loader),
for the third one updateProgressBar I'm not sure if it's needed anymore, I would love some feedback if that's the right way of implementing createProgressBar and removeProgressBar.

This PR follows uses vanilla JS without adding dependencies.

Testing Instructions

  1. Go to the Media Manager
  2. Select an image and click the "Edit" button
  3. Make any edit to the image (resize, crop, etc.)
  4. Click the "Save" button to upload the modified image
  5. Observe that the progress container is properly created before upload and removed after completion

Actual result BEFORE applying this Pull Request

Before this PR, the progress bar methods were only placeholders with TODO comments. While they were called during the upload process, they didn't properly create or remove the progress indicator elements.

Screencast.from.03-18-2025.03.13.04.PM.webm

Expected result AFTER applying this Pull Request

After applying this PR, the system now properly creates a container for the progress indicator before upload begins and correctly removes it when the upload completes or fails.

Screencast.from.03-18-2025.03.07.53.PM.webm

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.2-dev labels Mar 18, 2025
@mahmoudmagdy1-1 mahmoudmagdy1-1 changed the title Implement createProgressBar and removeProgressBar methods Implement progress bar for Media Manager image edits Mar 18, 2025
@mahmoudmagdy1-1 mahmoudmagdy1-1 changed the title Implement progress bar for Media Manager image edits [5.2] Implement progress bar for Media Manager image edits Mar 18, 2025
@laoneo
Copy link
Member

laoneo commented Mar 19, 2025

Are the eslint ignore lines still needed?

eslint-disable-next-line class-methods-use-this

@Fedik
Copy link
Member

Fedik commented Mar 19, 2025

The progress bar in the PR does not show the actual progress.
You also have to implement updateProgressBar(), otherwise it does not make sense.

@mahmoudmagdy1-1
Copy link
Author

Are the eslint ignore lines still needed?

eslint-disable-next-line class-methods-use-this

With the current implementation, the build fails when not including them as I'm not using this in the removeProgressBar

@mahmoudmagdy1-1
Copy link
Author

The progress bar in the PR does not show the actual progress. You also have to implement updateProgressBar(), otherwise it does not make sense.

This progess bar works just like the progress bar in the media manager, they both use media-loader class which doesn't show the actual progress.
However, I just made another implementation that adds a progress div that's hidden to the layout template

<div id="progress" class="progress visually-hidden">
    <div id="progress-bar" class="progress-bar bg-success" role="progressbar" aria-valuenow="0" aria-valuemin="0" aria-valuemax="100"></div>
</div>

and just toggle it visually when adding / deleting the progress bar

// eslint-disable-next-line class-methods-use-this
createProgressBar() {
  const progress = document.getElementById('progress');
  progress.classList.remove('visually-hidden');
}


// eslint-disable-next-line class-methods-use-this
updateProgressBar(position) {
  const progressBar = document.querySelector('.progress-bar');
  if (progressBar) {
    progressBar.style.width = `${position}%`;
    progressBar.setAttribute('aria-valuenow', position);
  }
}

// eslint-disable-next-line class-methods-use-this
removeProgressBar() {
  const progress = document.getElementById('progress');
  const progressBar = document.querySelector('.progress-bar');
  if (progress) {
    progress.classList.add('visually-hidden');
    progressBar.style.width = '0';
    progressBar.setAttribute('aria-valuenow', '0');
  }
}

This implementation actually shows the progress and utilizes the updateProgressBar function, this is how it looks

Screencast.from.03-19-2025.02.31.24.PM.webm

I'm not sure if this is the best way to go about solving this, but I would love some feedback, thanks!

@richard67
Copy link
Member

@mahmoudmagdy1-1 It seems you have changed the mode (Unix permissions) of file "build/media_source/com_media/js/edit-images.es6.js" from 644 to 755. Could you revert that?

@Fedik
Copy link
Member

Fedik commented Mar 19, 2025

This progess bar works just like the progress bar in the media manager, they both use media-loader class which doesn't show the actual progress.

I know, but that also was wrong, it does not give to User any information about how long it takes.
The animation take 1 sec, but the data processing could take a teens of seconds, or even minutes.
We should avoid such progress representation.

The upload progress will be fixed in #44848

However, I just made another implementation that adds a progress div that's hidden to the layout template

Yes, that probably will be much more useful.

@dgrammatiko
Copy link
Contributor

Are the eslint ignore lines still needed?
eslint-disable-next-line class-methods-use-this

With the current implementation, the build fails when not including them as I'm not using this in the removeProgressBar

You could change your code a bit and remove the eslint disable rule, ie:

createProgressBar() {
  this.progress = document.getElementById('progress');
  this.progressBar = document.querySelector('.progress-bar');
  this.progress.classList.remove('visually-hidden');
}

updateProgressBar(position) {
  if (this.progressBar) {
    this.progressBar.style.width = `${position}%`;
    this.progressBar.setAttribute('aria-valuenow', position);
  }
}


removeProgressBar() {
  if (this.progress && this.progressBar) {
    this.progress.classList.add('visually-hidden');
    this.progressBar.style.width = '0';
    this.progressBar.setAttribute('aria-valuenow', '0');
  }
}

@mahmoudmagdy1-1
Copy link
Author

Yes, that probably will be much more useful.

You could change your code a bit and remove the eslint disable rule, ie:

Thanks for the feedback, I'll make a commit now with these changes

@QuyTon
Copy link
Contributor

QuyTon commented Mar 25, 2025

Please rebase to the v5.3 branch. Thanks.

@mahmoudmagdy1-1 mahmoudmagdy1-1 changed the base branch from 5.2-dev to 5.3-dev March 25, 2025 02:22
@mahmoudmagdy1-1 mahmoudmagdy1-1 changed the title [5.2] Implement progress bar for Media Manager image edits [5.3] Implement progress bar for Media Manager image edits Mar 25, 2025
@QuyTon QuyTon removed the PR-5.2-dev label Mar 25, 2025
@richard67
Copy link
Member

shouldnt that be 6.0? When @HLeithner did the big rebase the other week all unmerged features were moved to 6

@brianteeman For now 5.4-dev is ok. We (maintainers and/or RMs) will decide soon if 5.4 or 6.0. If we decide for 6.0 it is easier to rebase again from 5.4-dev to 6.0-dev than it would be the other way around.

@richard67 richard67 added the RMDQ ReleaseManagerDecisionQueue label Mar 26, 2025
@brianteeman
Copy link
Contributor

@richard67 very confusing for contributors

@richard67
Copy link
Member

@richard67 very confusing for contributors

@brianteeman I know. But that should not last long, I hope we will have it clarified soon in general for feature PRs.

@brianteeman
Copy link
Contributor

especially as so many PR were already force moved to 6

@richard67 richard67 added Updates Requested Indicates that this pull request needs an update from the author and should not be tested. and removed RMDQ ReleaseManagerDecisionQueue labels Apr 13, 2025
@richard67 richard67 removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Apr 13, 2025
@muhme
Copy link
Contributor

muhme commented Apr 23, 2025

I have tested this item 🔴 unsuccessfully on 8340aad

Tested with current 5.4-dev, before this PR there is no progress bar. After applying this PR and running npm ci there is a big green progress bar on saving the image. There is no progress bar on image edits, like loading for crop, rotate or resize -> therefore this test fails.

It may be helpful to create test images, e.g. with:

  • 0,5 MB 400 x 400 px

    • convert -size 400x400 xc: +noise Random -depth 8 -gravity Center -pointsize 60 -fill black -stroke black -strokewidth 1 -annotate 0 "Test Image\n400x400" test-image-400.png
  • 50 MB 4.000 x 4.000 px

    • convert -size 4000x4000 xc: +noise Random -depth 8 -gravity Center -pointsize 600 -fill darkred -stroke black -strokewidth 4 -annotate 0 "Test Image\n4000x4000" test-image-4000.png
  • You may have to configure:

    • Joomla Media Maximum Size: 100 MB
    • PHP memory_limit = 1024M
    • PHP post_max_size = 100M
    • PHP upload_max_filesize = 100M

Additional the file upload progress bar is a small blue one and the image saving bar is a big green one. Should this not be visual consistency?

Screen shots and a video is uploaded to GitHub afterwards.


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

@muhme
Copy link
Contributor

muhme commented Apr 23, 2025

https://github.com/user-attachments/assets/d53d9316-f461-4f19-bbac-1edc436a84b9 shows

  • Opening a 50 MB image is missing progress bar
  • Rotate this image is missing progress bar
  • Resize joomla_black.png is missing progress bar

File/Image upload bar is small blue one:
upload-bar

Image saving bar is big green one:
save-bar

@muhme muhme added the RMDQ ReleaseManagerDecisionQueue label Apr 23, 2025
@softforge
Copy link
Contributor

Thank you @mahmoudmagdy1-1 so much for your work on this.
It was the main focus of the maintainers' meeting last night, and @muhme did an amazing job of going through it in detail.

Several things came out, one is that there is an inconsistency with the use, so for example you are proposing it for save but rotation and other editing effects also take time, and with @muhme demo they were sometimes in the order of 5x longer than the save, so it would be ideal to be consistent by adding the same progression to those events as well.

There was a discussion as to whether it should be the Joomla spinner that is shown to allow the user to know that something is happening. But the progress bar was agreed although the large green thick version did seem perhaps too bulky and different from progress we have in other places, the thin blue line.

A good point was raised about why implement it as you have, why not use the html element
https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/progress

This is widely supported now and would be the ideal way to semantically add it to a page.

If this were rebased to 6.0 (5.4 comes out the same time but is not designed for new features but a bridge between majors) then @Bodge-IT and I as the release managers for 6.0 would welcome it as a new feature and help as much as we can to get it accepted.

Look forward to your feedback

@muhme
Copy link
Contributor

muhme commented Apr 25, 2025

Thanks @softforge and after the maintainers advice to test it with throttled network connection, a new feature request was created, please see #45377. The slowest part is opening the image for editing.

@mahmoudmagdy1-1
Copy link
Author

mahmoudmagdy1-1 commented Apr 25, 2025

Thank you for testing the pr and your input, It's much appreciated.

There is no progress bar on image edits, like loading for crop, rotate or resize -> therefore this test fails.

We can't track real progress for "loading for crop, rotate or resize" because it happens synchronously, and there is no indication sent about the progress happening, but when saving we get xhr.upload.onprogress which sends the real progress that we can use to update the progress bar with real progress,
we can implement a progress bar for loading, that doesn't show the actual progress like the one in the media manager, Just to let users know It's loading.

image

the large green thick version did seem perhaps too bulky and different from progress we have in other places, the thin blue line.

the progress bar in the other places doesn't track the actual progress as Fedik mentioned, he also said that should be fixed with #44848.

why not use the html element

Sure I'm going to include this in my next commit, I'm just not too sure about how should I deal with the inconsistency of loading the image and the other editing effects.

@richard67 richard67 changed the base branch from 5.4-dev to 6.0-dev April 27, 2025 09:13
@richard67 richard67 changed the title [5.4] Implement progress bar for Media Manager image edits [6.0] Implement progress bar for Media Manager image edits Apr 27, 2025
@richard67 richard67 removed RMDQ ReleaseManagerDecisionQueue PR-5.4-dev labels Apr 27, 2025
@richard67
Copy link
Member

@mahmoudmagdy1-1 Based on the discussion in the maintainers team I have allowed myself to rebase your PR to 6.0. Let me know if that's not ok for you and I shall change it back.

@mahmoudmagdy1-1
Copy link
Author

mahmoudmagdy1-1 commented Apr 27, 2025

Let me know if that's not ok for you and I shall change it back.

It's fine for me

@Bodge-IT
Copy link
Contributor

Bodge-IT commented May 1, 2025

We really appreciate the effort @mahmoudmagdy1-1, but it has been decided not to move forward with this PR at the moment, as it introduces some visual inconsistencies. After #44848, it could be worth to work again on the image editing progress bar. Looking forward to your contributions in the future!

@Bodge-IT Bodge-IT closed this May 1, 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.