Skip to content
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.

Sign up for GitHub

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

Feature/3949/allow editing before upload #3988

Merged
merged 12 commits into from
Aug 17, 2020
Merged

Feature/3949/allow editing before upload #3988

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
722fe35
Store current upload id
marcoambrosini Jul 28, 2020
6ef9611
Store visibility of the upload editor
marcoambrosini Jul 28, 2020
661348e
Add uploadeditor component
marcoambrosini Jul 28, 2020
07fee78
Create initialiseduploads store entry and split uploadfiles logic
marcoambrosini Aug 3, 2020
551305d
Add ability to add more files to the current upload
marcoambrosini Aug 3, 2020
d116643
Make the filepreview component compatible with upload editing
marcoambrosini Aug 3, 2020
2915171
Add ability to remove files from selection
marcoambrosini Aug 5, 2020
ef09c4c
Show previews in the upload editor
marcoambrosini Aug 5, 2020
9e6e2d5
Move share logic to the uploadfiles method in the store
marcoambrosini Aug 5, 2020
c49ee85
Style upload editor
marcoambrosini Aug 5, 2020
d738442
Use unique random indexes for temporary messages
marcoambrosini Aug 10, 2020
6653eb6
Make uploadeditor accessible
marcoambrosini Aug 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 43 additions & 19 deletions src/components/MessagesList/MessagesGroup/Message/MessagePart/FilePreview.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
<!--
- @copyright Copyright (c) 2019 Joas Schilling <coding@schilljs.com>
- @copyright Copyright (c) 2020 Marco Ambrosini <marcoambrosini@pm.me>
-
- @author Joas Schilling <coding@schilljs.com>
- @author Marco Ambrosini <marcoambrosini@pm.me>
-
- @license GNU AGPL version 3 or any later version
-
Expand All @@ -20,11 +22,9 @@
-->

<template>
<a :href="link"
<file-preview v-bind="filePreview"
Copy link
Member

Choose a reason for hiding this comment

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

This file really bothers me. The shared part is actually so little, that it should just be 2 maintainable subcomponents instead.

class="container"
:class="{ 'is-viewer-available': isViewerAvailable }"
target="_blank"
rel="noopener noreferrer"
:class="{ 'is-viewer-available': isViewerAvailable, 'upload-editor': isUploadEditor }"
@click="showPreview">
<img v-if="!isLoading && !failed"
:class="previewSizeClass"
Expand All @@ -37,8 +37,8 @@
<span v-if="isLoading"
class="preview loading" />
<strong>{{ name }}</strong>
<ProgressBar v-if="isTemporaryUpload" :value="uploadProgress" />
</a>
<ProgressBar v-if="isTemporaryUpload && !isUploadEditor" :value="uploadProgress" />
</file-preview>
</template>

<script>
Expand Down Expand Up @@ -97,6 +97,11 @@ export default {
type: String,
default: '',
},
// True if this component is used in the upload editor
isUploadEditor: {
type: Boolean,
default: false,
},
},
data() {
return {
Expand All @@ -105,6 +110,23 @@ export default {
}
},
computed: {
// This is used to decide which outer element type to use
// a or div
filePreview() {
if (this.isUploadEditor || this.isTemporaryUpload) {
return {
is: 'div',
tag: 'div',
}
}
return {
is: 'a',
tag: 'a',
href: this.link,
target: '_blank',
rel: 'noopener noreferrer',
}
},
defaultIconUrl() {
return imagePath('core', 'filetypes/file')
},
Expand Down Expand Up @@ -213,20 +235,14 @@ export default {
image. */
display: inline-block;

/* Show a hover colour around the preview when navigating with the
* keyboard through the file links (or hovering them with the mouse). */
&:hover,
&:focus,
&:active {
.preview {
background-color: var(--color-background-hover);
border-radius: 16px;

/* Trick to keep the same position while adding a padding to show
* the background. */
box-sizing: content-box !important;
padding: 10px;
margin: -10px;
}
&:hover,
&:focus {
background-color: var(--color-background-hover);
/* Trick to keep the same position while adding a padding to show
* the background. */
box-sizing: content-box !important;
Copy link
Member

Choose a reason for hiding this comment

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

While this looks good in the upload editor in my opinion the look in the messages list has regressed. Therefore I would suggest to have both the previous style and the new one, and use one or the other depending on whether the upload-editor class is set or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think has regressed in particular? Before there was an unneeded hover feedback on the preview itself that partially covered the sender's username.

Copy link
Member

Choose a reason for hiding this comment

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

there was an unneeded hover feedback on the preview itself

I think that the hover feedback should be there, as it shows the user that she can interact with the file. The problem is that the whole message is "highlighted" on hover when showing the reply button, and that hides the specific feedback for the file. Anyway, I would be fine with removing the hover feedback on the file due to that overlap (but I would prefer to find a better solution nevertheless).

that partially covered the sender's username

I know, it has been haunting me since I added it :-P

However, even if it covers the sender's username I prefer that small, focused frame on the file:
Message-File-Focus-Before

Rather than highlighting the whole message body (even if it actually matches the real clickable area):
Message-File-Focus-After

For me the previous version feels and looks much better when navigating with the keyboard. Also the change is not directly related to editing the upload, so in any case I would address it in a separate pull request too.

Copy link
Member Author

Choose a reason for hiding this comment

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

So this is the current behavior (before this pr):
Peek 2020-08-13 09-43

So you get the big hover feedback of the message but no clickability unless you hover the preview, and since the hover color is the same, you get the hover feedback of the preview only through unwanted stuff:

  • overflow from the message on the left;
  • invading the name on top;

The keyboard navigation is a bug with how messages hover feedback is implemented. For a reference of how it should really look refer to the mouse navigation feedback. Plus, I think that a bigger click area is generally better, since that space is unused anyway... I implemented the change in this pr because I'm adapting the filepreview component.

Peek 2020-08-13 09-31

}

.preview {
Expand All @@ -244,6 +260,8 @@ export default {
/* As the file preview is an inline block the name is set as a block to
force it to be on its own line below the preview. */
display: block;
overflow: hidden;
white-space: nowrap;
}

&:not(.is-viewer-available) {
Expand All @@ -253,4 +271,10 @@ export default {
}
}

.upload-editor {
max-width: 160px;
max-height: 160px;
margin: 10px;
}

</style>