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.

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/translate modal #4299

Merged
merged 2 commits into from
Jul 25, 2023
Merged

Feature/translate modal #4299

merged 2 commits into from
Jul 25, 2023

Conversation

luka-nextcloud
Copy link
Contributor

@luka-nextcloud luka-nextcloud commented Jun 15, 2023

📝 Summary

🖼️ Screenshots

Desktop
grafik
Mobile1 Mobile2
grafik grafik

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@cypress
Copy link

cypress bot commented Jun 15, 2023

Passing run #11310 ↗︎

0 149 2 0 Flakiness 0

Details:

Feature/translate modal
Project: Text Commit: 8a2b20ab2e
Status: Passed Duration: 03:52 💡
Started: Jul 25, 2023 8:55 PM Ended: Jul 25, 2023 8:59 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@juliusknorr
Copy link
Member

Follow up bugs to file:

  • Line break are not preserved when selecing multiple paragraphs

@juliusknorr
Copy link
Member

Pulled in designers for visual review ;)

@luka-nextcloud luka-nextcloud force-pushed the feature/translate-modal branch from 809765e to e49fe81 Compare June 16, 2023 08:05
Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

Nice work, I just dropped a few style suggestions.

Maybe we could also change the hint into something like:
You can translate parts of the document by selecting the text before using the translate function.

src/components/Modal/Translate.vue Outdated Show resolved Hide resolved
src/components/Modal/Translate.vue Show resolved Hide resolved
@juliusknorr juliusknorr added enhancement New feature or request design Experience, interaction, interface, … labels Jun 19, 2023
@luka-nextcloud luka-nextcloud force-pushed the feature/translate-modal branch 2 times, most recently from be308c4 to 8e0afc2 Compare June 20, 2023 13:10
@juliusknorr
Copy link
Member

juliusknorr commented Jun 20, 2023

A few more small issues that I think would be worth to address:

  • Limit the maximum height of the text areas so that the buttons at the bottom are still visible without scrolling
  • The result text area should probably still be scrollable as currently the translated text is being cut off if it is longer then the source
    Screenshot 2023-06-20 at 15 17 50

@nimishavijay
Copy link
Member

Looks good! :) I would be careful about small screens here, it should change to a single column layout.

Also a note on the labels: "source language" and "target language" seem a bit technical, and on Google Translate as well as Deepl, there are no labels at all.

  • What do you think about using "Translate from" and "Translate to"?
  • Alternatively, we could get rid of labels entirely, make the multiselects to full width and use a right arrow icon in between them to indicate the source and target language.

What do you think @nextcloud/designers ?

@luka-nextcloud luka-nextcloud force-pushed the feature/translate-modal branch from 8e0afc2 to 8e25311 Compare June 21, 2023 15:45
@juliusknorr juliusknorr requested a review from mejo- June 26, 2023 12:13
@juliusknorr
Copy link
Member

@mejo- Could you give this a review? I'd say we could merge this already and do a follow up for the remaining open question to be discussed by the design team.

Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Nice work @luka-nextcloud!

Could you address the two issues mentioned by Julius here? Both textareas should have overflow-y: auto and the buttons should always be visible, even with small window height.

Some extra comments:

  • I don't find it very intuitive that I have to press 'Translate' after both source and target language have been selected. Why not remove the button and instead start translating immediately after both source and target have been selected? What do you think @nimishavijay and @marcoambrosini?
  • On mobile view, label and selection of languages are not aligned properly due to different label length. Maybe always put them in separate lines on mobile? See screenshot:
    grafik

src/components/Modal/Translate.vue Outdated Show resolved Hide resolved
src/components/Modal/Translate.vue Outdated Show resolved Hide resolved
src/components/Modal/Translate.vue Outdated Show resolved Hide resolved
src/components/Modal/Translate.vue Outdated Show resolved Hide resolved
src/components/Modal/Translate.vue Show resolved Hide resolved
src/components/Modal/Translate.vue Outdated Show resolved Hide resolved
@juliusknorr
Copy link
Member

I don't find it very intuitive that I have to press 'Translate' after both source and target language have been selected. Why not remove the button and instead start translating immediately after both source and target have been selected?

Especially with the local translation model this can take quite some time and server resources so unless we can always ensure a fast response I'd try to avoid auto triggering that request

@nimishavijay
Copy link
Member

On mobile view, label and selection of languages are not aligned properly due to different label length. Maybe always put them in separate lines on mobile?

True! There definitely needs to be some sort of rearrangement on mobile. We could either:

  1. Show the labels and dropdowns on separate lines
  2. Shorten the labels
  3. Not show external labels at all, instead show internal labels "Translate from German" "Translate to English" inside the dropdowns in the closed state and make the dropdowns full width.

I would vote to show solution 3 (only on mobile), what do you think? @nextcloud/designers

@luka-nextcloud luka-nextcloud force-pushed the feature/translate-modal branch from 8e25311 to a7bbf77 Compare June 29, 2023 14:13
@luka-nextcloud luka-nextcloud requested a review from mejo- June 30, 2023 14:25
@mejo-
Copy link
Member

mejo- commented Jul 12, 2023

@luka-nextcloud we're almost there, but some requested changes are still not addressed 😉:

  • Limit the maximum height of the text areas so that the buttons at the bottom are still visible without scrolling (as mentioned by @juliushaertl here) -> Still broken when the viewport height is less than 850px. Try it e.g. with height of 800px.
  • [Only on mobile] Not show external labels at all, instead show internal labels "Translate from German" "Translate to English" inside the dropdowns in the closed state and make the dropdowns full width. (as mentioned by @nimishavijay here)

@luka-nextcloud luka-nextcloud force-pushed the feature/translate-modal branch from a7bbf77 to d8e366d Compare July 13, 2023 19:28
@luka-nextcloud
Copy link
Contributor Author

@mejo- Please check again.

src/components/Modal/Translate.vue Outdated Show resolved Hide resolved
src/components/Modal/Translate.vue Outdated Show resolved Hide resolved
@luka-nextcloud luka-nextcloud force-pushed the feature/translate-modal branch from d8e366d to 2850f46 Compare July 25, 2023 08:31
@luka-nextcloud luka-nextcloud requested a review from mejo- July 25, 2023 08:32
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Nice, good to go from my side ☺️

@mejo-
Copy link
Member

mejo- commented Jul 25, 2023

/compile

Signed-off-by: nextcloud-command <[email protected]>
@jancborchardt
Copy link
Member

Are the screenshots current or could you post updated ones @luka-nextcloud?
Ideally both for desktop (side by side layout) and mobile (single column layout). :)

@mejo-
Copy link
Member

mejo- commented Jul 25, 2023

@jancborchardt @luka-nextcloud I took the liberty to now update the screenshots in the original PR description.

@mejo- mejo- merged commit ce17203 into main Jul 25, 2023
@jancborchardt jancborchardt deleted the feature/translate-modal branch July 27, 2023 15:15
@thgoebel
Copy link

Is this released? For me the Translate popup still looks like in this screenshot (i.e. it is not side-by-side).

  • Firefox 117.0.1 on a laptop, I tried cleaning the browser cache
  • Nextcloud Server 27.1.0.7
  • Text 3.8.0 (according to /settings/apps). This is the same as when I was on Server 27.0.0, is this expected?

@mejo-
Copy link
Member

mejo- commented Sep 20, 2023

@thgoebel this PR didn't get backported to Nextcloud 27. It will be released with Nextcloud 28.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review design Experience, interaction, interface, … enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Translate design feedback
8 participants