Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-4712 Add progress bar to uploads #2010

Merged
merged 4 commits into from
Nov 21, 2018
Merged

MM-4712 Add progress bar to uploads #2010

merged 4 commits into from
Nov 21, 2018

Conversation

sudheerDev
Copy link
Contributor

@sudheerDev sudheerDev commented Nov 6, 2018 •
edited
Loading

Summary

Add progress bar to uploads as per spec

Ticket Link

(MM-4712)[https://mattermost.atlassian.net/browse/MM-4712]

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Ran make check-style to check for style errors (required for all pull requests)
  • Ran make test to ensure unit and component tests passed
  • Added or updated unit tests (required for all new features)
  • Has UI changes
  • Includes text changes and localization file (.../i18n/en.json) updates

nov-06-2018 20-36-04

@sudheerDev sudheerDev added the 1: PM Review Requires review by a product manager label Nov 6, 2018
@sudheerDev sudheerDev added the Setup Old Test Server Triggers the creation of a test server label Nov 6, 2018
@esethna esethna self-assigned this Nov 6, 2018
esethna
esethna suggested changes Nov 6, 2018
View reviewed changes
Copy link
Contributor

@esethna esethna left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks Sudheer! A few comments:

  1. The progress bar should not have rounded edges, it should fill the bounding box

image

Design:
image

  1. The "Uploading... (%)" and "processing" text should be grey not black. See design for exact color and text style

  2. There should be a space between the "..." and "(X%)"

image

  1. Some file types not recognized show as a blank thumbnail until it's loaded:

image

^ This reproduced the first time I tried to load a PDF as well. Might have to do with uploading a file type for the first time if the icon is not in cache and the file is large?

@esethna esethna added this to the v5.6.0 milestone Nov 6, 2018
@esethna esethna added the Awaiting Submitter Action Blocked on the author label Nov 6, 2018
@sudheerDev
Copy link
Contributor Author

@esethna Fixed 1,2,3

Re: 4. That should be because of loading pdf icon for the first time .
If we cannot detect the file we present another generic default icon so there wont be a case of having empty space there

@esethna esethna removed the Setup Old Test Server Triggers the creation of a test server label Nov 8, 2018
@mattermost mattermost deleted a comment from mattermod Nov 8, 2018
@mattermost mattermost deleted a comment from mattermod Nov 8, 2018
@mattermost mattermost deleted a comment from mattermod Nov 8, 2018
@esethna esethna added the Setup Old Test Server Triggers the creation of a test server label Nov 8, 2018
@mattermod
Copy link
Contributor

Setup Test Server label detected. Spinmint test server created if build succeeds (checks pass and no conflicts with base branch).

@mattermod
Copy link
Contributor

Spinmint test server created at: https://i-0506b46bb96c6c932.test.spinmint.com

Test Admin Account: Username: sysadmin | Password: sysadmin

Test User Account: Username: user-1 | Password: user-1

Instance ID: i-0506b46bb96c6c932

@esethna
Copy link
Contributor

esethna commented Nov 8, 2018 •
edited
Loading

@sudheerDev sorry there should be no space between the number and the % sign. ie 40%

image

Other than that looks good

@esethna esethna added the 2: Dev Review Requires review by a core commiter label Nov 8, 2018
@esethna esethna removed 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server labels Nov 8, 2018
@mattermod
Copy link
Contributor

Spinmint test server destroyed

@esethna esethna removed their assignment Nov 8, 2018
@sudheerDev sudheerDev removed the Awaiting Submitter Action Blocked on the author label Nov 12, 2018
@sudheerDev
Copy link
Contributor Author

@esethna Changes done.

hmhealey
hmhealey suggested changes Nov 13, 2018
View reviewed changes
components/file_preview.jsx Outdated
const fileType = getFileTypeFromMime(fileInfo.extension);
previewImage = <div className={'file-icon ' + Utils.getIconClassName(fileType)}/>;

fileNameComponent = (
Copy link
Member

Choose a reason for hiding this comment

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

Could this be refactored out into its own component? That might make it easier to test on its own

components/file_preview.jsx Outdated
<span>{percentTxt}</span>
</React.Fragment>
)}

Copy link
Member

Choose a reason for hiding this comment

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

There's a random blank line that should be removed

utils/file_utils.jsx Outdated Show resolved Hide resolved
@esethna
Copy link
Contributor

esethna commented Nov 19, 2018

@hmhealey @sudheerDev what's next steps on this PR?

@sudheerDev
Copy link
Contributor Author

sudheerDev commented Nov 19, 2018 •
edited
Loading

@hmhealey @sudheerDev what's next steps on this PR?

Waiting for HH to go through the changes for another review

@hmhealey hmhealey added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Nov 21, 2018
@sudheerDev sudheerDev merged commit 1d5d3ea into mattermost:master Nov 21, 2018
@sudheerDev sudheerDev deleted the MM-4712 branch November 21, 2018 19:54
@amyblais amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Nov 22, 2018
@lindy65 lindy65 added Tests/Done Release tests have been written and removed 4: Reviews Complete All reviewers have approved the pull request labels Nov 28, 2018
@JtheBAB JtheBAB mentioned this pull request Jan 15, 2019
Closed
9 tasks
@snyk-bot snyk-bot mentioned this pull request Apr 7, 2022
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hmhealey hmhealey hmhealey approved these changes

@saturninoabril saturninoabril saturninoabril approved these changes

@esethna esethna esethna approved these changes

Assignees
No one assigned
Labels
Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation Tests/Done Release tests have been written
Projects
None yet
Milestone
v5.6.0
7 participants
@sudheerDev @mattermod @esethna @hmhealey @saturninoabril @lindy65 @amyblais