Replace spinner with Progress bar and fix it's position . (#5620) #5929

victorchrollo14 · 2024-03-27T16:09:34Z

Fixes #5620.

Replaced the spinner in contents -> upload, With a progress bar that indicates the number of files uploaded. ( Uploaded x out of y files ).
Fixed the bad positioning of the spinner when a large number of files were uploaded by add a max-height and overflow: auto which fixes issue ( File upload modal needs overflow: auto and max-height #5677 ).

To Implement the progress bar.

Added a new property of uploadedFiles to the contents redux state.
Added a 'UPDATE_UPLOADED_FILES' type to ActionTypes.js, an action creator and a reducer function for the same.
Added a local totalFiles state inside ContetsUploadModal.
To update the number of files uploaded we dispatch the updateUploadedFile action inside the redux api middleware, after each of the upload requests are fullfilled. and finally set the uploadedFiles to 0 after all the requests are fulfilled.
Added unit tests for the updateUploadedFile action creator and the reducer as well.

Screencast.from.26-03-24.06.57.09.PM.IST.webm

…g large number of files. (plone#5620)

netlify · 2024-03-27T16:09:51Z

✅ Deploy Preview for volto canceled.

Name	Link
🔨 Latest commit	`9d8a3f3`
🔍 Latest deploy log	https://app.netlify.com/sites/volto/deploys/663649aa98b79f000820aa22

netlify · 2024-03-27T16:09:51Z

✅ Deploy Preview for plone-components canceled.

Name	Link
🔨 Latest commit	`9d8a3f3`
🔍 Latest deploy log	https://app.netlify.com/sites/plone-components/deploys/663649aaa09ac50008f95aa3

stevepiercy · 2024-03-27T21:19:48Z

I restarted the failed CI check, and we'll see if it passes this time.

stevepiercy

That's great that you took the extra effort to provide translations to all those languages. I'll request review from the suggested folks, but anyone who has an interest in getting this merged is welcome to add a review. Thank you!

stevepiercy · 2024-03-27T21:25:22Z

CI is all green. Let's get a review from @plone/volto-team.

victorchrollo14 · 2024-03-28T00:33:44Z

That's great that you took the extra effort to provide translations to all those languages. I'll request review from the suggested folks, but anyone who has an interest in getting this merged is welcome to add a review. Thank you!

Hey @stevepiercy. I didn't add the translations, the earlier message of 'uploading files' got automatically removed after I made the changes, I thought this is normal and committed those changes.

I'll add the translations and push it up then.

ichim-david · 2024-03-29T11:43:17Z

packages/volto/src/components/manage/Contents/ContentsUploadModal.jsx

+              className="progress-bar"
+              value={this.props.uploadedFiles}
+              total={this.state.totalFiles}
+            >{`Uploading: ${this.props.uploadedFiles} Files Out Of ${this.state.totalFiles}`}</Progress>


@victorchrollo14 you need to add the message with formatMessage using several attributes.
Here is an example of how it's done within Sharing.jsx
https://github.com/plone/volto/blob/main/packages/volto/src/components/manage/Sharing/Sharing.jsx#L90
https://github.com/plone/volto/blob/main/packages/volto/src/components/manage/Sharing/Sharing.jsx#L434
and how it looks when the message is extracted
https://github.com/plone/volto/blob/main/packages/volto/locales/en/LC_MESSAGES/volto.po#L407

We do not want the message to be hardcoded to english since Volto is a multi language framework.

yeah @stevepiercy mentioned that, I'll add that up.

… contentsUploadmodal

packages/volto/locales/ca/LC_MESSAGES/volto.po

packages/volto/locales/de/LC_MESSAGES/volto.po

packages/volto/news/5620.bugfix

stevepiercy

Messages look good now. Let's get one more review from a @plone/volto-team member.

ichim-david · 2024-04-01T20:12:24Z

@victorchrollo14 the feature looks like it works based on your recording. I wouldn't be surprised if you won't receive feedback to tweak the styles or for us todo so after merging.
I would still like to test it and see how it performs and it might take a while until this is merged but I have a good feeling about this one, congratulations.

victorchrollo14 · 2024-04-02T01:17:16Z

@victorchrollo14 the feature looks like it works based on your recording. I wouldn't be surprised if you won't receive feedback to tweak the styles or for us todo so after merging.
I would still like to test it and see how it performs and it might take a while until this is merged but I have a good feeling about this one, congratulations.

I just came up with a random implementation and was expecting someone would tell me to implement it in a better way or tweak it a bit.

I too am surprised that I was not asked to make changes in code. I'll fix it up if there are any issues.

avoinea

The Romanian translation looks good.

ichim-david · 2024-04-02T10:44:17Z

packages/volto/src/middleware/api.js

@@ -188,6 +188,7 @@ const apiMiddlewareFactory =
                    ),
                  },
                ).then((reqres) => {
+                  dispatch(updateUploadedFiles(uploadedFiles++));


@davisagli @sneridagh Some input from you guys on the technical aspect would be more than welcomed, I know that as an "end-user", we like what we see but I wonder how you feel about this implementation.
I was talking with Tiberiu and a comment that he had was something in the lines of:
this dispatch is always running with the assumption that if this is true
? mode === 'serial'
? request.reduce((prevPromise, item)
Right now indeed it's used only the image upload but in the future it might be used for something else as well so maybe it's not good to have this simplistic assumption.
This might be a non factor but again some technical eyes would be more than welcomed I think.

@ichim-david I'd say, let's narrow the dispatch to the upload files action.

I can see we are using a SUBREQUEST = 'batch-upload'. Let's use this.

@victorchrollo14 could you give it a try?

@sneridagh. I'm not sure if I'm getting it right.
Please correct me if I'm wrong.
What you are telling is to move the uploadedFiles property into the content.subrequests['batch-upload'] such that the the upload files action would accept a subrequest parameter

export function updateUploadedFiles(uploadedFiles, subrequest) { return { type: UPDATE_UPLOADED_FILES, subrequest, uploadedFiles: uploadedFiles, };

while dispatching we would pass it as follows

dispatch(updateUploadedFiles(++uploadedFiles, 'batch-upload')),

we would update the subrequests['batch-upload'].uploadedFiles inside the reducer.
and use this value inside the contentsUploadModal.

sneridagh

@victorchrollo14 please look at the comment. BTW, very nice to the eye.

sneridagh · 2024-04-03T10:55:41Z

packages/volto/src/middleware/api.js

@@ -188,6 +188,7 @@ const apiMiddlewareFactory =
                    ),
                  },
                ).then((reqres) => {
+                  dispatch(updateUploadedFiles(uploadedFiles++));


@ichim-david I'd say, let's narrow the dispatch to the upload files action.

I can see we are using a SUBREQUEST = 'batch-upload'. Let's use this.

@victorchrollo14 could you give it a try?

ichim-david

@victorchrollo14 @plone/volto-team I don't like the way the the text is
so janky as the counter increments and there is very poor contrast of the
text due to the gray overlay with the blue text.
I propose the following changes found in my gist
https://gist.github.com/ichim-david/0756e28aa1999d5d29ff7ebeede93413

And my changes would make the upload look and function as such.

my-proposal-styling-bg-white.mp4

If someone has a better solution of different color combination now's the time to speak or forever hold your tongue :)

EDIT:
In my video there is a small or close to small overlap when we have extra 0 such as 200. I've bumped up the width to 2.25 and now the issue is gone. Some of you guys might complain about having too much space to take into account the potential 3 digit file upload.
Either someone comes up with a better css styling or maybe we need to pad the numbers with zeroes so if we have 50 it starts 01 if we upload 100+ it starts with 001 as a solution to avoid large spacing between the words and the incrementer.

stevepiercy · 2024-04-30T19:24:12Z

I dislike the spacing around the counter. I think it is OK to have the text jump as this happens in every progress bar I have ever watched, but if not, then I would left-align the text and replace it with two lines:

Files uploaded: <count>
Total files to upload: <total>

I agree that the contrast is too low between the counter text and its grey overlay background. However I think that a background color on the entire progress indicator container would be the better option. The rounded corners of both the counter text container and the progress bar stacked on top of each other is inconsistent with Pastanaga's principles of no rounded corners.

Finally, I think the progress bar itself should be rectangular without rounded corners.

ichim-david · 2024-04-30T19:42:59Z

I dislike the spacing around the counter. I think it is OK to have the text jump as this happens in every progress bar I have ever watched, but if not, then I would left-align the text and replace it with two lines:
Files uploaded: <count>
Total files to upload: <total>
I agree that the contrast is too low between the counter text and its grey overlay background. However I think that a background color on the entire progress indicator container would be the better option. The rounded corners of both the counter text container and the progress bar stacked on top of each other is inconsistent with Pastanaga's principles of no rounded corners.

Finally, I think the progress bar itself should be rectangular without rounded corners.

I like the 2 line solution and it would get rid of the spacing and counter padding needed and since it's the last text the previous text would probably remain in place.

I am fine with having a bg color for that section and getting rid of the rounded option if this is desired.
@stevepiercy you might need to make a screenshot with your proposal so that we know exactly what you want for bg color

stevepiercy · 2024-04-30T20:10:40Z

@stevepiercy you might need to make a screenshot with your proposal so that we know exactly what you want for bg color

As long as there is sufficient contrast, I'm OK with whatever is chosen. White appears to be the default background color, so I would go with that. However there might be an existing style or class that can be reused instead of creating a new one. blocks-chooser may be a good class to reuse.

djay · 2024-05-01T04:57:36Z

I'm not saying there is a need to change anything UI wise but quanta does have a design for how this should look.

If there are changes to the UI then it would be nice to make them in the direction of the quanta design (layout, not styles)

Some differences are

the order of the columns
putting the preview and name in the same column
last modified date (coming from filesystem)
putting the DND target at the bottom of the list
not using an overlay
Using the sidebar for finalising or cancelling the upload
obviously styles/colours but that isn't possible until a later version

Quanta doesn't say much about the progress bar looks but we did an implementation of upload screen that worked with TUS that which means that it gives progress on individual files (in case they are large). Not sure that can be done here as you might not get single file progress using the normal upload api?

The details for how we proposed the final upload UI would look are all in
#5423

In any case, just a heads up this UI may change again in the future as the proposal was to switch to TUS for all uploads

stevepiercy · 2024-05-01T05:48:53Z

There are two other Quanta related PLIPs:

They are targeted for Plone 7 and depend on other parts that are beyond the scope of fixing this bug. I'd prefer to fix the bug now, reusing styles that already exist.

ichim-david · 2024-05-01T06:03:55Z

@stevepiercy @djay we have to take into consideration also the scope of this ticket which is to simply provide a progress bar with some info on how many files.

I took Steve's suggestion and tweaked the rounded corners of the progress and added the info on 2 lines.
I think this is a good solution but let's hear your opinion.

flex-two-lines.mp4

stevepiercy · 2024-05-01T06:25:09Z

@ichim-david the contrast is sufficient, and there's no more jumpy text. I think it is good. Would you please push a commit with your work?

I think we're waiting for input from @nileshgulia1.

djay · 2024-05-01T06:32:28Z

To be clear I wasn't referring to quanta styles but the layout and functionality.

ichim-david · 2024-05-01T08:07:58Z

packages/volto/theme/themes/pastanaga/extras/contents.less

+  .progress-bar {
+    width: 300px;
+    background: #e4e8ec;
+
+    .bar {
+      background: #0074a3;
+    }
+
+    .label {
+      margin-top: 5px;
+      color: #0074a3;
+      font-size: medium;
+      font-weight: normal;
+    }
+  }
+}


Suggested change

.progress-bar {

width: 300px;

background: #e4e8ec;

.bar {

background: #0074a3;

}

.label {

margin-top: 5px;

color: #0074a3;

font-size: medium;

font-weight: normal;

}

}

}

.progress-bar {

width: 300px;

background: #e4e8ec;

border-radius: 4px 4px 0 0;

.bar {

background: #0074a3;

border-radius: 4px 4px 0 0;

}

.label {

color: #0074a3;

font-size: medium;

font-weight: normal;

background-color: rgba(255, 255, 255, 0.7);

padding: 0.5em;

margin-top: -1px;

border-radius: 0 0 4px 4px;

display: flex;

flex-wrap: wrap;

gap: 0.5rem;

}

}

}

ichim-david · 2024-05-01T08:11:59Z

packages/volto/src/components/manage/Contents/ContentsUploadModal.jsx

+  uploadingFiles: {
+    id: 'Uploading {uploadedFiles} files out of {totalFiles}',
+    defaultMessage: 'Uploading {uploadedFiles} files out of {totalFiles}',
+  },


Suggested change

uploadingFiles: {

id: 'Uploading {uploadedFiles} files out of {totalFiles}',

defaultMessage: 'Uploading {uploadedFiles} files out of {totalFiles}',

},

filesUploaded: {

id: 'Files uploaded: ',

defaultMessage: 'Files uploaded: ',

},

totalFilesToUpload: {

id: 'Total files to upload: {totalFiles}',

defaultMessage: 'Total files to upload: {totalFiles}',

},

ichim-david · 2024-05-01T08:12:41Z

packages/volto/src/components/manage/Contents/ContentsUploadModal.jsx

+              {this.props.intl.formatMessage(messages.uploadingFiles, {
+                uploadedFiles: this.props.uploadedFiles,
+                totalFiles: this.state.totalFiles,
+              })}


Suggested change

{this.props.intl.formatMessage(messages.uploadingFiles, {

uploadedFiles: this.props.uploadedFiles,

totalFiles: this.state.totalFiles,

})}

{this.props.intl.formatMessage(messages.filesUploaded)}

<span className="uploaded-files">{this.props.uploadedFiles}</span>

<br />

{this.props.intl.formatMessage(messages.totalFilesToUpload, {

totalFiles: this.state.totalFiles,

})}

ichim-david

@victorchrollo14 I added as suggestions that you can batch and commit in a single commit the changes I suggest to the styling and the text content.
You will need to run pnpm y18n command to update the translations po files and use the latest translations messages.

packages/volto/locales/de/LC_MESSAGES/volto.po

victorchrollo14 · 2024-05-01T09:57:28Z

@stevepiercy @djay we have to take into consideration also the scope of this ticket which is to simply provide a progress bar with some info on how many files.

I took Steve's suggestion and tweaked the rounded corners of the progress and added the info on 2 lines. I think this is a good solution but let's hear your opinion.

flex-two-lines.mp4

@ichim-david @stevepiercy @djay @sneridagh @nileshgulia1. Can we try something like this.
if this doesn't feel fine, then I'll make the changes @ichim-david suggested.

Screencast.from.01-05-24.03.10.15.PM.IST.webm

stevepiercy · 2024-05-01T10:04:34Z

@victorchrollo14 absolutely, positively, 100% YES! That is exactly what I imagined, but could not put into words. It looks consistent, accessible, and well-integrated.

victorchrollo14 · 2024-05-02T02:31:33Z

@victorchrollo14 absolutely, positively, 100% YES! That is exactly what I imagined, but could not put into words. It looks consistent, accessible, and well-integrated.

I'll push these changes then.

…4/volto into loader-issue-5620

ichim-david

@victorchrollo14 I'm fine now with the changes, thank you for your work and the willingness to go through the necessary round of changes in order to make something even better than when it started as this is the strength of the open source community where you can receive feedback from several members and each can contribute to the successful outcome.

Co-authored-by: Katja Süss <[email protected]>

* main: (1206 commits) Allow Makefile to be modified by Makefile.local if present (#5997) Cypress replaced wait with assertions (#5998) Replace spinner with Progress bar and fix it's position . (#5620) (#5929) Fix type for component registry components (#6002) Fix 301 and 302 redirects. (#5999) Release 18.0.0-alpha.30 Force add .d.ts files to releaser Release generate-volto 9.0.0-alpha.15 Fix server side sidebar rendering (#5993) Cleaned up useless injectIntl in DefaultView (#5995) Fix image disappears after pressing the Enter key on title field in i… (#5975) Update to Plone 6.0.11 (#5989) Defines the last 4 parameters of the `asyncConnect` function with optional (#5986) Flexibilize the pins for all ESlint deps, in Volto and generators (#5991) Release 18.0.0-alpha.29 Release @plone/types 1.0.0-alpha.11 fix: pass down locale to IntlProvider (#5976) Add Vite (client only, no SSR) build. Update Next.js 14.2.2 and Remix to 2.8.0 (#5970) Fix no router link in logo (#5981) Improve postinstall script, building all the deps (#5980) ...

* main: Revert "Fix 301 and 302 redirects. (#5999)" (#6008) Pin eslint-plugin-jsx-a11y to ^6.7.1 everywhere it's referenced (#5794) Allow Makefile to be modified by Makefile.local if present (#5997) Cypress replaced wait with assertions (#5998) Replace spinner with Progress bar and fix it's position . (#5620) (#5929)

Replace spinner with Progress bar and fix it's position when uploadin…

4cf9431

…g large number of files. (plone#5620)

stevepiercy reviewed Mar 27, 2024

View reviewed changes

stevepiercy requested review from sneridagh, erral, iFlameing and tisto March 27, 2024 21:24

giuliaghisini approved these changes Mar 28, 2024

View reviewed changes

ichim-david requested changes Mar 29, 2024

View reviewed changes

victorchrollo14 added 2 commits March 30, 2024 19:43

Merge remote-tracking branch 'upstream/main' into loader-issue-5620

fe42dd6

add internationalization and translation to uploadingFiles message in…

f99afce

… contentsUploadmodal

stevepiercy requested changes Mar 30, 2024

View reviewed changes

packages/volto/locales/ca/LC_MESSAGES/volto.po Outdated Show resolved Hide resolved

packages/volto/locales/de/LC_MESSAGES/volto.po Outdated Show resolved Hide resolved

packages/volto/news/5620.bugfix Outdated Show resolved Hide resolved

victorchrollo14 added 3 commits March 31, 2024 13:16

Merge remote-tracking branch 'upstream/main' into loader-issue-5620

88b8940

Merge remote-tracking branch 'upstream/main' into loader-issue-5620

a4f73ee

fix sentence casing issues in translations and changelog in 5620.bugfix

151e66a

stevepiercy approved these changes Mar 31, 2024

View reviewed changes

stevepiercy requested review from ichim-david and a team March 31, 2024 20:43

stevepiercy added 24 status: ready 32 needs: review labels Mar 31, 2024

avoinea approved these changes Apr 2, 2024

View reviewed changes

ichim-david reviewed Apr 2, 2024

View reviewed changes

sneridagh requested changes Apr 3, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into loader-issue-5620

a5f6156

ichim-david requested changes Apr 30, 2024

View reviewed changes

ichim-david mentioned this pull request Apr 30, 2024

Add tabindex to each of the skiplink elements to better handle focus #5959

Merged

stevepiercy requested a review from nileshgulia1 May 1, 2024 06:23

nileshgulia1 approved these changes May 1, 2024

View reviewed changes

ichim-david reviewed May 1, 2024

View reviewed changes

ichim-david requested changes May 1, 2024

View reviewed changes

ksuess requested changes May 1, 2024

View reviewed changes

packages/volto/locales/de/LC_MESSAGES/volto.po Outdated Show resolved Hide resolved

victorchrollo14 added 3 commits May 2, 2024 20:37

Merge remote-tracking branch 'upstream/main' into loader-issue-5620

f349257

changed the ui of the progress bar

e4d462a

Merge branch 'loader-issue-5620' of https://github.com/victorchrollo1…

250723e

…4/volto into loader-issue-5620

ichim-david self-requested a review May 3, 2024 10:27

ichim-david approved these changes May 3, 2024

View reviewed changes

Update packages/volto/locales/de/LC_MESSAGES/volto.po

9d8a3f3

Co-authored-by: Katja Süss <[email protected]>

sneridagh approved these changes May 4, 2024

View reviewed changes

sneridagh merged commit 0307e86 into plone:main May 6, 2024
47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace spinner with Progress bar and fix it's position . (#5620) #5929

Replace spinner with Progress bar and fix it's position . (#5620) #5929

victorchrollo14 commented Mar 27, 2024

netlify bot commented Mar 27, 2024 •

edited

Loading

netlify bot commented Mar 27, 2024 •

edited

Loading

stevepiercy commented Mar 27, 2024

stevepiercy left a comment

stevepiercy commented Mar 27, 2024

victorchrollo14 commented Mar 28, 2024

ichim-david Mar 29, 2024

victorchrollo14 Mar 29, 2024

stevepiercy left a comment

ichim-david commented Apr 1, 2024

victorchrollo14 commented Apr 2, 2024

avoinea left a comment

ichim-david Apr 2, 2024

sneridagh Apr 3, 2024

victorchrollo14 Apr 3, 2024

sneridagh left a comment

sneridagh Apr 3, 2024

ichim-david left a comment •

edited

Loading

stevepiercy commented Apr 30, 2024

ichim-david commented Apr 30, 2024

stevepiercy commented Apr 30, 2024

djay commented May 1, 2024 •

edited

Loading

stevepiercy commented May 1, 2024

ichim-david commented May 1, 2024

stevepiercy commented May 1, 2024

djay commented May 1, 2024

ichim-david May 1, 2024

ichim-david May 1, 2024

ichim-david May 1, 2024

ichim-david left a comment

victorchrollo14 commented May 1, 2024

stevepiercy commented May 1, 2024

victorchrollo14 commented May 2, 2024

ichim-david left a comment

-  uploadingFiles: {
-    id: 'Uploading {uploadedFiles} files out of {totalFiles}',
-    defaultMessage: 'Uploading {uploadedFiles} files out of {totalFiles}',
-  },
+   filesUploaded: {
+    id: 'Files uploaded: ',
+    defaultMessage: 'Files uploaded: ',
+  },
+  totalFilesToUpload: {
+    id: 'Total files to upload: {totalFiles}',
+    defaultMessage: 'Total files to upload: {totalFiles}',
+  },

Replace spinner with Progress bar and fix it's position . (#5620) #5929

Replace spinner with Progress bar and fix it's position . (#5620) #5929

Conversation

victorchrollo14 commented Mar 27, 2024

To Implement the progress bar.

netlify bot commented Mar 27, 2024 • edited Loading

✅ Deploy Preview for volto canceled.

netlify bot commented Mar 27, 2024 • edited Loading

✅ Deploy Preview for plone-components canceled.

stevepiercy commented Mar 27, 2024

stevepiercy left a comment

Choose a reason for hiding this comment

stevepiercy commented Mar 27, 2024

victorchrollo14 commented Mar 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevepiercy left a comment

Choose a reason for hiding this comment

ichim-david commented Apr 1, 2024

victorchrollo14 commented Apr 2, 2024

avoinea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sneridagh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ichim-david left a comment • edited Loading

Choose a reason for hiding this comment

stevepiercy commented Apr 30, 2024

ichim-david commented Apr 30, 2024

stevepiercy commented Apr 30, 2024

djay commented May 1, 2024 • edited Loading

stevepiercy commented May 1, 2024

ichim-david commented May 1, 2024

stevepiercy commented May 1, 2024

djay commented May 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ichim-david left a comment

Choose a reason for hiding this comment

victorchrollo14 commented May 1, 2024

stevepiercy commented May 1, 2024

victorchrollo14 commented May 2, 2024

ichim-david left a comment

Choose a reason for hiding this comment

netlify bot commented Mar 27, 2024 •

edited

Loading

netlify bot commented Mar 27, 2024 •

edited

Loading

ichim-david left a comment •

edited

Loading

djay commented May 1, 2024 •

edited

Loading