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

File drop UI fixes and improvements #5505

Merged
merged 37 commits into from
Mar 5, 2021

Conversation

SimonBrandner
Copy link
Contributor

@SimonBrandner SimonBrandner commented Dec 17, 2020

This PR fixes the misplaced UI and fixes #3541 along with improving the UI a bit with a background for better readability. It follows the slight adjustments suggested in this design.

Peek 2021-02-25 18-50
Peek 2021-02-26 08-13

Note: The GIFs are a bit laggy. Use the live preview for a better experience

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@@ -42,6 +42,7 @@ const RoomContext = createContext<IState>({
canReply: false,
useIRCLayout: false,
matrixClientIsReady: false,
dragCounter: 0,
Copy link
Contributor Author

@SimonBrandner SimonBrandner Dec 17, 2020

Choose a reason for hiding this comment

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

This is probably not necessary here? But there is draggingFile: false, is that intentional?

Copy link
Member

Choose a reason for hiding this comment

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

I'm slightly worried that counting enters/leaves will cause problems and desync (there's a ton of ways to leave a browser window without triggering a dragleave event, making you always +1)

Can we get away with a simple isDragging instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is the only solution I've found... I don't think I quite understand what you mean by isDragging here. There seems to be a isDragging method in react-dnd but I don't think this can be used here. What am I missing?

@t3chguy t3chguy requested review from a team December 29, 2020 12:49
@nadonomy
Copy link
Contributor

@SimonBrandner apologies for the laggy review on this— trying to resource some design time to improve the colours and visual treatment of this interaction but overall looking great!

@niquewoodhouse
Copy link
Contributor

Hi @SimonBrandner I took a quick look at this and have a proposal. I think I understood the video properly, but as always, please let me know if I misunderstood or in practice this won't work because of reasons I've not considered. In some quick testing, I couldn't quite work out the logic for how the existing backgrounds appear, and I don't want to accidentally introduce new logic. My proposal is something like this:

ele-web-upload-1

  • Amend the white background which appears over the timeline in dark theme
  • Remove the background of the inner box (which contains the upload icon and text)
  • Add feedback to the icon when file is in correct place to be dropped**
  • Add feedback upon drop**

** = Actually, I'm not sure if these are actually required. Adding feedback might be hard to do in the existing logic, and the subsequent tick might look ok but its confusing because currently a dialog appears to confirm uploading, the tick might suggest it's done when it's not.

So my proposal is just to change the first two and give the user this view:

  • Dark theme

2 1@2x

  • Light theme

2 1-light@2x

Figma

The upload icon should exist already but let me know if not/you can't get it from the figma/if you have any questions.
Thanks

@SimonBrandner
Copy link
Contributor Author

Hi @niquewoodhouse, I think the check would be a bit weird given the current UI. I'd stick with the simple design at least for now. I'll look into implementing what you have suggested!

@SimonBrandner SimonBrandner marked this pull request as draft February 25, 2021 17:13
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner SimonBrandner marked this pull request as ready for review February 25, 2021 18:10
@SimonBrandner
Copy link
Contributor Author

SimonBrandner commented Feb 25, 2021

@niquewoodhouse, done!

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
This seemed to have caused a little lag and it was unnecessary

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner SimonBrandner marked this pull request as draft February 26, 2021 09:52
We need to allow the container to be smaller

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner SimonBrandner marked this pull request as ready for review February 26, 2021 10:32
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

overall looks fine, just a couple clarifying questions. Thanks!

src/components/structures/RoomView.tsx Outdated Show resolved Hide resolved
src/components/views/rooms/AuxPanel.tsx Outdated Show resolved Hide resolved
src/components/structures/RoomView.tsx Outdated Show resolved Hide resolved
@@ -42,6 +42,7 @@ const RoomContext = createContext<IState>({
canReply: false,
useIRCLayout: false,
matrixClientIsReady: false,
dragCounter: 0,
Copy link
Member

Choose a reason for hiding this comment

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

I'm slightly worried that counting enters/leaves will cause problems and desync (there's a ton of ways to leave a browser window without triggering a dragleave event, making you always +1)

Can we get away with a simple isDragging instead?

SimonBrandner and others added 5 commits March 2, 2021 07:41
Co-authored-by: Travis Ralston <travpc@gmail.com>
Co-authored-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner SimonBrandner requested review from turt2live and removed request for a team March 3, 2021 06:49
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

thanks!

@turt2live turt2live merged commit 676259e into matrix-org:develop Mar 5, 2021
@SimonBrandner SimonBrandner deleted the improve-file-drop-ui branch March 5, 2021 06:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop file here to upload flashes
4 participants