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

Jump to bottom

"Share with" wording fix in new share panel #41155

Closed
wants to merge 3 commits into from
Closed

"Share with" wording fix in new share panel #41155

wants to merge 3 commits into from

Conversation

Jerome-Herbinet
Copy link
Member

@Jerome-Herbinet Jerome-Herbinet commented Oct 27, 2023 •
edited
Loading

Summary

  • Removing space ; so we get "Share with" instead of "Share with " and adding space after translation call (because, IMO a space at the end of a wording is not a good idea)
  • Replacing "Share" with "Shared", because, IMO, it's more relevant and consistent (isn't it ?)

Note : I require a syntax checking concerning the moved space character.

Checklist

@Jerome-Herbinet
Update SharingDetailsTab.vue
4702f0b 
Signed-off-by: Jérôme Herbinet <33763786+Jerome-Herbinet@users.noreply.github.com>
@Jerome-Herbinet
npm run build
b9bc4df 
Signed-off-by: Jérôme Herbinet <33763786+Jerome-Herbinet@users.noreply.github.com>
@Jerome-Herbinet Jerome-Herbinet requested review from Pytal, blizzz and nfebe and removed request for Pytal and blizzz October 27, 2023 11:52
@Jerome-Herbinet Jerome-Herbinet added 3. to review Waiting for reviews feature: language l10n and translations feature: file sidebar Related to the file sidebar component labels Oct 27, 2023
susnux
susnux reviewed Oct 27, 2023
View reviewed changes
apps/files_sharing/src/views/SharingDetailsTab.vue Outdated
@@ -292,7 +292,7 @@ export default {

computed: {
title() {
let title = t('files_sharing', 'Share with ')
let title = t('files_sharing', 'Shared with') + ' ';
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it not make more sense to have full separate strings for every option?
I can imagine there are lanuages that do not work lik "share with" + " " + "option".
E.g. Japanese does not work like that, so it will be pretty hard to translate.

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes sense. Let's wait for other reviewers' opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @rakekniven

Copy link
Member

@rakekniven rakekniven Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@susnux See my comment below. A screenshot would help a lot.

nfebe
nfebe reviewed Oct 27, 2023
View reviewed changes
Copy link
Contributor

@nfebe nfebe left a comment

Choose a reason for hiding this comment

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

Shared would not be true for new shares. It should be Share when user has not saved share and Shared otherwise

apps/files_sharing/src/views/SharingDetailsTab.vue Outdated
@@ -292,7 +292,7 @@ export default {

computed: {
title() {
let title = t('files_sharing', 'Share with ')
let title = t('files_sharing', 'Shared with') + ' ';
Copy link
Contributor

@nfebe nfebe Oct 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Fine with taking out the space.

As for the word Shared it would have to be dynamic, because that would not be true for new shares.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right but in this case, I won't have the required skills to do such changes ; my skills scope is CSS design and wordings calls change.

Copy link
Member

Choose a reason for hiding this comment

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

@fenn-cs could you adjust it so it works in both cases? :)

@solracsf solracsf added this to the Nextcloud 28 milestone Oct 29, 2023
@rakekniven
Copy link
Member

Can someone please add a screenshot here?
I was unable to find the occurence of given string in web UI using NC v27.1.3 .

@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
Merged
@Jerome-Herbinet
Copy link
Member Author

Can someone please add a screenshot here? I was unable to find the occurence of given string in web UI using NC v27.1.3 .

@rakekniven :

2023-11-02_14-05

@jancborchardt
Copy link
Member

Both in that heading and in the button, the word "share" is a noun, so it’s correct to have it called "share" and not "shared". :)

@nfebe
Copy link
Contributor

nfebe commented Nov 2, 2023

@jancborchardt So what is the conclusion? So I can implement it.

@susnux
fixup!: Ensure that the string can be translated independed of senten…
7dee136 
…ce order of language

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
Copy link
Contributor

susnux commented Nov 2, 2023

Sorry to tackle your PR but I pushed a commit to show what I mean, because this will allow to translate the sentence independent of sentence order of the language. Feels free to drop that commit if not happy with it :)

This was referenced Nov 6, 2023
Merged
Merged
This was referenced Nov 14, 2023
Merged
Merged
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
This was referenced Mar 12, 2024
Merged
Merged
Merged
This was referenced Mar 20, 2024
Merged
Merged
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
Merged
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@Jerome-Herbinet
Copy link
Member Author

If I understand well, this PR hasn't been merged into Nextcloud 29 ; what was the problem ?

This was referenced Jul 30, 2024
Merged
Merged
@Altahrim Altahrim mentioned this pull request Aug 5, 2024
Merged
@skjnldsv skjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Aug 6, 2024
@Altahrim Altahrim mentioned this pull request Aug 7, 2024
Merged
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
Merged
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
@Jerome-Herbinet
Copy link
Member Author

@jancborchardt it's fixed in NC 29 (maybe since 28 and maybe in NC 30 I hope = no regression) so I close my PR which is useless now.

@jancborchardt jancborchardt deleted the Jerome-Herbinet-share-with-fix branch August 22, 2024 14:47
@jancborchardt
Copy link
Member

@Jerome-Herbinet yep, sorry that it didn’t come with your pull request, but let’s be happy it was fixed either way. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nfebe nfebe nfebe left review comments

@rakekniven rakekniven rakekniven left review comments

@susnux susnux susnux left review comments

@Pytal Pytal Awaiting requested review from Pytal

@blizzz blizzz Awaiting requested review from blizzz

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@nimishavijay nimishavijay Awaiting requested review from nimishavijay

Assignees
No one assigned
Labels
2. developing Work in progress feature: file sidebar Related to the file sidebar component feature: language l10n and translations stale Ticket or PR with no recent activity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Jerome-Herbinet @rakekniven @jancborchardt @nfebe @susnux @blizzz @solracsf @skjnldsv