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

feat: adds ability to copy data from across locales #8203

Open
wants to merge 35 commits into
base: beta
Choose a base branch
from

Conversation

JessChowdhury
Copy link
Member

@JessChowdhury JessChowdhury commented Sep 13, 2024

  1. Adds ability to copy data from one locale to another from UI
  2. Adds e2e tests to localization test suite

Feature tracked here.

Screenshot 2024-09-13 at 11 47 53 AM

Copy link
Contributor

@DanRibbens DanRibbens left a comment

Choose a reason for hiding this comment

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

Looks good other than the error handling and translation todo. Nice work!

packages/ui/src/elements/CopyLocaleData/index.tsx Outdated Show resolved Hide resolved
packages/ui/src/elements/CopyLocaleData/index.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@DanRibbens DanRibbens left a comment

Choose a reason for hiding this comment

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

I was going ot merge this but then I noticed there are a handful of missing translations to fix.
unsavedChanges: undefined,

Do you want to take care of that before I merge?

@JessChowdhury
Copy link
Member Author

I was going ot merge this but then I noticed there are a handful of missing translations to fix. unsavedChanges: undefined,

Do you want to take care of that before I merge?

@DanRibbens undefined translations have been fixed 👍

@denolfe denolfe self-assigned this Oct 9, 2024
Copy link
Contributor

@DanRibbens DanRibbens left a comment

Choose a reason for hiding this comment

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

I tried to test the feature with blocks and it errored for me.
This was taken from our test/localization suite working with the Nested Arrays collection.
image
I'm not sure if this is because the collection has versions enabled or if it was nested data that couldn't be handled.

Aside from this, I have UX concerns about this feature as it would be too easy for a user to click the wrong locale and overwrite some existing data. It feels to me like we should have a modal or something to add an additional step for acknowledging that they might be overwriting everything in a document on the target locale.

The copy button should look more like a secondary actions with dropdown options like this:
image

@JessChowdhury
Copy link
Member Author

I tried to test the feature with blocks and it errored for me. This was taken from our test/localization suite working with the Nested Arrays collection. image I'm not sure if this is because the collection has versions enabled or if it was nested data that couldn't be handled.

Aside from this, I have UX concerns about this feature as it would be too easy for a user to click the wrong locale and overwrite some existing data. It feels to me like we should have a modal or something to add an additional step for acknowledging that they might be overwriting everything in a document on the target locale.

The copy button should look more like a secondary actions with dropdown options like this: image

Changes made:

  1. Fix copying any nested or relationship fields
  2. Improve button UI
  3. Add confirmation modal before copying
  4. Redirect to new locale

Copy link
Contributor

@DanRibbens DanRibbens left a comment

Choose a reason for hiding this comment

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

Ah sorry I didn't manage to get this feedback on the last review.

packages/translations/src/languages/en.ts Outdated Show resolved Hide resolved
packages/ui/src/elements/CopyLocaleData/index.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@DanRibbens DanRibbens left a comment

Choose a reason for hiding this comment

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

I found another issue with the feature. If you have a document that has 0 localized fields, we still show the copy to locale controls.

Copy link
Contributor

@DanRibbens DanRibbens left a comment

Choose a reason for hiding this comment

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

I found two more issues while reviewing this. Pardon me if you're still working on these details, thought I'd better share while you're still working on it!

  1. When I copy with nested blocks it is no longer erroring, but the data doesn't copy in a blocks field. The locale data is just empty in the target locale after a "successful" copy.
    Before copy:
    Screenshot 2024-10-17 160552
    After:
    Screenshot 2024-10-17 160607

  2. When editing a doc with drafts enabled, I don't have the Copy button.
    image

@JessChowdhury
Copy link
Member Author

JessChowdhury commented Oct 21, 2024

I found two more issues while reviewing this. Pardon me if you're still working on these details, thought I'd better share while you're still working on it!

  1. When I copy with nested blocks it is no longer erroring, but the data doesn't copy in a blocks field. The locale data is just empty in the target locale after a "successful" copy.
    Before copy:
    After:
  2. When editing a doc with drafts enabled, I don't have the Copy button.

Hey @DanRibbens these two issues are fixed and PR ready for review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants