Skip to content

Fix(MessageButtonsBar): Hide copy message button for files #10840

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
Merged
nickvergessen merged 1 commit into from
Nov 7, 2023
Merged

Fix(MessageButtonsBar): Hide copy message button for files #10840

nickvergessen merged 1 commit into from
Nov 7, 2023

Conversation

@DorraJaouad
Copy link
Contributor

@DorraJaouad DorraJaouad commented Nov 6, 2023 •
edited
Loading

☑️ Resolves

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏚️ Before 🏡 After
image image

🚧 Tasks

  • Code review

🏁 Checklist

@DorraJaouad DorraJaouad added this to the 💙 Next Beta (28) milestone Nov 6, 2023
@DorraJaouad DorraJaouad requested a review from ShGKme November 6, 2023 08:03
@DorraJaouad DorraJaouad self-assigned this Nov 6, 2023
nickvergessen
nickvergessen reviewed Nov 6, 2023
View reviewed changes
src/components/MessagesList/MessagesGroup/Message/MessageButtonsBar/MessageButtonsBar.vue Outdated
readableTime: key,
timeLocale: moment?.format('LT'),
}),
ariaLabel: t('spreed', `Set reminder for ${key.toLowerCase()}`),
Copy link
Member

@nickvergessen nickvergessen Nov 6, 2023

Choose a reason for hiding this comment

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

This does not work for translations. Please revert to old pattern with fixed strings.

See https://docs.nextcloud.com/server/latest/developer_manual/basics/front-end/l10n.html#important-notes for more information

Copy link
Contributor Author

@DorraJaouad DorraJaouad Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The aria label is translatable but I am not sure if the text label is translatable. I think that's where the translation broke ?
image

Anyway, for such cases, how to translate strings with variable part that should be translated ?

Copy link
Member

@nickvergessen nickvergessen Nov 6, 2023

Choose a reason for hiding this comment

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

Anyway, for such cases, how to translate strings with variable part that should be translated ?

This is called string concatenation and does not work in all languages. Just don't use variabled strings. Go back to the predefined array that was here. (It's also unrelated to "Hide copy message button")

Copy link
Contributor Author

@DorraJaouad DorraJaouad Nov 6, 2023

Choose a reason for hiding this comment

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

Yes, it was just an attempt to reduce the redundancy

@DorraJaouad DorraJaouad force-pushed the fix/10829/hide-copy-formatted-message branch from dd3f7f4 to a9ef89b Compare November 6, 2023 13:25
@nickvergessen nickvergessen merged commit fa3e04f into main Nov 7, 2023
@nickvergessen nickvergessen deleted the fix/10829/hide-copy-formatted-message branch November 7, 2023 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@ShGKme ShGKme ShGKme approved these changes

Assignees

@DorraJaouad DorraJaouad

Labels

3. to review feature: chat 💬 Chat and system messages feature: frontend 🖌️ "Web UI" client

Projects

None yet

Milestone

v18.0.0-beta.2

Development

Successfully merging this pull request may close these issues.

Files without caption allow copying the message

4 participants

@DorraJaouad @nickvergessen @ShGKme