Skip to content

Enh/beta improvements #41481

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

Merged
szaimen merged 4 commits into from
Nov 22, 2023
Merged

Enh/beta improvements #41481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cd26b91
Move starring into actions menu
marcoambrosini Nov 15, 2023
eefbf6d
Verticaly align comment avatar
marcoambrosini Nov 15, 2023
0da90ec
Remove quotes from input
marcoambrosini Nov 15, 2023
f476220
Add star indicator in file description
marcoambrosini Nov 15, 2023
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
4 changes: 2 additions & 2 deletions apps/comments/src/components/Comment.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@
</div>
</div>
<div id="tab-comments__editor-description" class="comment__editor-description">
{{ t('comments', '"@" for mentions, ":" for emoji, "/" for smart picker') }}
{{ t('comments', '@ for mentions, : for emoji, / for smart picker') }}
</div>
</form>

Expand Down Expand Up @@ -288,7 +288,7 @@ $comment-padding: 10px;
&__side {
display: flex;
align-items: flex-start;
padding-top: 16px;
padding-top: 6px;
}

&__body {
Expand Down
20 changes: 17 additions & 3 deletions apps/files/src/views/Sidebar.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
tabindex="0"
@close="close"
@update:active="setActiveTab"
@update:starred="toggleStarred"
@[defaultActionListener].stop.prevent="onDefaultAction"
@opening="handleOpening"
@opened="handleOpened"
Expand All @@ -50,6 +49,16 @@

<!-- Actions menu -->
<template v-if="fileInfo" #secondary-actions>
<NcActionButton :close-after-click="true"
@click="toggleStarred(!fileInfo.isFavourited)">
<template v-if="fileInfo.isFavourited" #icon>
<StarOutline :size="20" />
</template>
<template v-else #icon>
<Star :size="20" />
</template>
{{ fileInfo.isFavourited ? t('files', 'Add to favorites') : t('files', 'Remove from favorites') }}
</NcActionButton>
<!-- TODO: create proper api for apps to register actions
And inject themselves here. -->
<NcActionButton v-if="isSystemTagsEnabled"
Expand Down Expand Up @@ -98,6 +107,9 @@ import $ from 'jquery'
import axios from '@nextcloud/axios'
import moment from '@nextcloud/moment'

import Star from 'vue-material-design-icons/Star.vue'
import StarOutline from 'vue-material-design-icons/StarOutline.vue'

import NcAppSidebar from '@nextcloud/vue/dist/Components/NcAppSidebar.js'
import NcActionButton from '@nextcloud/vue/dist/Components/NcActionButton.js'
import NcEmptyContent from '@nextcloud/vue/dist/Components/NcEmptyContent.js'
Expand All @@ -117,6 +129,8 @@ export default {
NcEmptyContent,
SidebarTab,
SystemTags,
Star,
StarOutline,
},

data() {
Expand Down Expand Up @@ -189,7 +203,8 @@ export default {
* @return {string}
*/
subtitle() {
return `${this.size}, ${this.time}`
const starredIndicator = this.fileInfo.isFavourited ? '★ ' : ''
Copy link
Contributor

@susnux susnux Nov 15, 2023

Choose a reason for hiding this comment

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

Suggested change
const starredIndicator = this.fileInfo.isFavourited ? '★ ' : ''
const starredIndicator = this.fileInfo.isFavourited ? '⭐ ' : ''

If we want to make it more discoverable / prominent that the file is a favorite we might want to use this instead.

Copy link
Member

@AndyScherzinger AndyScherzinger Nov 15, 2023

Choose a reason for hiding this comment

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

Is this accessible? The grey one would meet the contrast requirements I would hope.

CC @JuliaKirschenheuter

Copy link
Member

@jancborchardt jancborchardt Nov 15, 2023

Choose a reason for hiding this comment

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

I think we would have to use an icon with alt text / aria-label, right @JuliaKirschenheuter @marcoambrosini?

Copy link
Contributor

@susnux susnux Nov 15, 2023

Choose a reason for hiding this comment

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

Well we have --icon-starred-yellow as an icon that fulfills accessibility requirements for bright and dark mode.

Copy link
Member Author

@marcoambrosini marcoambrosini Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The problem is that the NcAppSidebar only accepts a string in that subline. I suggest we add the text "favorite" beside the star, so a screen reader would read out the info too.
Contrast is ok, yes.

Copy link
Member Author

@marcoambrosini marcoambrosini Nov 17, 2023

Choose a reason for hiding this comment

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

Also not possible, there it's either the switch or nothing. I suggest we go ahead with this and we modify the sidebar component so that we can slot in a proper icon there as @susnux suggests

Copy link
Member

@jancborchardt jancborchardt Nov 22, 2023

Choose a reason for hiding this comment

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

Also not possible, there it's either the switch or nothing.

Why is this not possible? Since the current solution is not accessible ("star" is not understandable as "favorite", and we can not add an aria-label) we need to do it otherwise.

Can we please do:

  • Just use an icon on the top right of the filetype icon, non-interactive like in the file list
  • The action to toggle it would be in the action menu, like you did here

@marcoambrosini @szaimen

Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter Nov 22, 2023

Choose a reason for hiding this comment

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

Just use an icon on the top right of the filetype icon, non-interactive like in the file list

Screenshot from 2023-11-22 13-44-55

and we have already everything we need

Copy link
Contributor

@ShGKme ShGKme Nov 22, 2023

Choose a reason for hiding this comment

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

Since the current solution is not accessible ("star" is not understandable as "favorite", and we can not add an aria-label) we need to do it otherwise.

I don't think that this is not accessible.

Emoji is read by a screen reader. It could be unclear that "star" means "favorite". But it is the same for all users. Any user needs to understand that "star" is "a starred file" which means "favorite".

This would be good to have a description here. But not required if we appeal to a common user experience of "starring".

Copy link
Member

@jancborchardt jancborchardt Nov 22, 2023

Choose a reason for hiding this comment

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

Ok, thanks for the assessment @ShGKme – then @AndyScherzinger and I would agree we should merge this as-is for now.

In the long term however it could possibly be better to have an image with aria-label which is more explicit, which we could also place on the top right of the filetype icon so it is symmetric to the files list.

return `${starredIndicator} ${this.size}, ${this.time}`
},

/**
Expand Down Expand Up @@ -246,7 +261,6 @@ export default {
},
compact: this.hasLowHeight || !this.fileInfo.hasPreview || this.isFullScreen,
loading: this.loading,
starred: this.fileInfo.isFavourited,
Copy link
Contributor

@susnux susnux Nov 15, 2023

Choose a reason for hiding this comment

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

We should then decide if we simply want to drop this in the library as well.

subname: this.subtitle,
subtitle: this.fullTime,
name: this.fileInfo.name,
Expand Down
4 changes: 2 additions & 2 deletions dist/7255-7255.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/7255-7255.js.map
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions dist/7856-7856.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/7856-7856.js.map
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions dist/comments-comments-app.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/comments-comments-app.js.map
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions dist/comments-comments-tab.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/comments-comments-tab.js.map
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions dist/core-common.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/core-common.js.map
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions dist/files-sidebar.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/files-sidebar.js.map
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions dist/files_sharing-files_sharing_tab.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/files_sharing-files_sharing_tab.js.map
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions dist/weather_status-weather-status.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/weather_status-weather-status.js.map
View file
Open in desktop

Large diffs are not rendered by default.