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

Allow the translation of enum value used for elementLabelProp in ExpandPanelRenderer #2289

Conversation

Maxouwell
Copy link
Contributor

PR for #2288

Copy link

netlify bot commented Feb 19, 2024

Deploy Preview for jsonforms-examples ready!

Name Link
🔨 Latest commit 4284d6e
🔍 Latest deploy log https://app.netlify.com/sites/jsonforms-examples/deploys/66212991139e7e00088d46ed
😎 Deploy Preview https://deploy-preview-2289--jsonforms-examples.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@sdirix sdirix 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 already. However we should also support the one of enum case

@coveralls
Copy link

coveralls commented Feb 20, 2024

Coverage Status

coverage: 83.269% (-1.6%) from 84.881%
when pulling 4284d6e on Maxouwell:fix/material-array-layout-translated-enum-label-prop
into 24f4e5f on eclipsesource:master.

@Maxouwell Maxouwell force-pushed the fix/material-array-layout-translated-enum-label-prop branch from 938e125 to 42ef5c1 Compare February 20, 2024 11:03
@Maxouwell
Copy link
Contributor Author

Maxouwell commented Feb 20, 2024

Resolved the conflicts, rebased and squashed

Maxouwell and others added 2 commits February 27, 2024 14:40
…label-prop

# Conflicts:
#	packages/material-renderers/test/renderers/MaterialArrayLayout.test.tsx
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

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

I had a look, improved the types and aligned the function declarations to arrow style as the rest of the code base.

I also noticed the following behavior.

Assume the following JSON Schema in use:

{
  type: 'object',
  properties: {
    comments: {
      type: 'array',
      title: 'Messages',
      items: {
        type: 'object',
        properties: {
          message1: {
            type: 'string',
          },
          message2: {
            type: 'string',
            enum: ['foo', 'bar'],
          },
        },
      },
    },
  },
}

Then the auto generated keys for the enum will be comments.message2.foo and comments.message2.bar.

However for the summary title in the expand panel, the following keys will be used: comments.foo and comments.bar. Therefore if a proper translation of the enum is used, it will not automatically show up in the expand panel, instead another key needs to be additionally translated.

I assume that this was not on purpose?

Can you fix the issue? Ideally the test case should also be improved. In the moment there is just a mock translator in there which does not check the handed over keys. I assume that is why the issue was overlooked.

@Maxouwell
Copy link
Contributor Author

You were right, the previous code had problems on translation key, and on elementLabelProp on deeper path.

I fixed it and added some tests for the deep cases.

@Maxouwell Maxouwell requested a review from sdirix March 8, 2024 17:51
Copy link
Member

@sdirix sdirix 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 one minor and one major issue with the current state. Besides that it looks good!

@Maxouwell Maxouwell requested a review from sdirix March 12, 2024 14:49
@Maxouwell
Copy link
Contributor Author

I just saw that the same behaviour exist in ListWithDetail, and that the solution don't support if elementLabelProp point to an enum array (or oneOf array if it's a thing).
I propose we finish this PR and I will open others PRs for these other ones.

@sdirix
Copy link
Member

sdirix commented Apr 15, 2024

Update: I will re-review this PR again this week

packages/core/src/util/label.ts Outdated Show resolved Hide resolved
packages/core/src/util/schema.ts Outdated Show resolved Hide resolved
packages/core/src/util/schema.ts Outdated Show resolved Hide resolved
@sdirix sdirix merged commit a672127 into eclipsesource:master Apr 18, 2024
8 checks passed
@Maxouwell Maxouwell deleted the fix/material-array-layout-translated-enum-label-prop branch April 18, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React MUI : Allow the translation of enum value used for elementLabelProp
3 participants