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
Show all details on mobile when showProgressDetails
is true
#3174
Conversation
…s in `StatusBar.js`
* main: Downgrade to Node 14 in GitHub Actions (#3179)
@@ -50,7 +50,6 @@ PRs are welcome! Please do open an issue to discuss first if it's a big feature, | |||
- [ ] Display data like image resolution on file cards. should be done by thumbnail generator maybe #783 | |||
- [ ] Possibility to edit/delete more than one file at once. example: add copyrigh info to 1000 files #118, #97 | |||
- [ ] Possibility to work on already uploaded / in progress files. We'll just provide the `fileId` to the `file-edit-complete` event so that folks can more easily roll out custom code for this themselves #112, #113, #2063 | |||
- [ ] Show upload speed too if `showProgressDetails: true`. Maybe have separate options for which things are displayed, or at least have css-classes that can be hidden with `display: none` #766 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't you check the box instead of removing the line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we have a huge backlog I figured removing might be best to unclutter things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFACT the code is working, I've left a few comments to discuss the coding style (I think it could improved here and there, but if you disagree maybe that shouldn't block this PR).
Good points. I mostly focused on making the structure and flow better and mostly copy pasted code snippets around. Resulting in me not even seeing the obvious improvements like redundant if statements for booleans, or other lines of code. Regarding exports at the bottom, I don't feel too strongly about it, but from a readability perspective I always preferred it at the top so the order becomes:
This way, you get an immediate overview of everything a file does. Whereas if you put functions on top, and exports at the bottom, you have to go through implementation details even if you just wanted to understand the flow or keypoints of this file. |
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
The upside of having exports at the end of the file is:
I don't feel too strongly either, but I think we should aim for consistency: either move all exports to the middle, or all to the bottom. |
This reverts commit de823a6.
* main: Show all details on mobile when `showProgressDetails` is `true` (#3174) @uppt/xhr-upload: fix `this.uppy is undefined` error (#3207) ci: test on Node.js v16.x (#3205) @uppy/dashboard: fix linter (#3206) Add `2.1.1` to `CHANGELOG.md` Release Release Fix "attempted to use private field on non-instance" in `SearchProvider` (#3201) Add 'done' to `nb_NO.js` (#3200) @uppy/transloadit: fix unhandledPromiseRejection failures (#3197) fix: AbortController is not defined on Node.js (Server Side Render) (#3169)
Fixes #766
Allow hoisted functions in
.eslintrc
I'm strongly in favor of using
use-before-define
, but ignoring it for functions. Hoisting is a feature which can be used to write more intention revealing code.Refactor status bar plugin and resolve all eslint warnings
I took small steps with commits in between to greatly simplify the code and make it more obvious what is happening. See
StatusBar
before and after.Show all details on mobile when
showProgressDetails
istrue
See this commit if you only want to see the changes relevant to this fix.