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

Enhancement/2122 add description to array renderer #2227

Merged
merged 7 commits into from
Jan 10, 2024
Merged

Enhancement/2122 add description to array renderer #2227

merged 7 commits into from
Jan 10, 2024

Conversation

SaSteffen
Copy link
Contributor

As discussed in #2122: This is a minimal change that renders a description for primitive and object array renderers in React MUI.

The Primitive example looks like this:
image

The other one looks like this:
image

As discussed, I just used a FormHelperText. We could achieve similar results using custom typography elements probably.

@CLAassistant
Copy link

CLAassistant commented Dec 12, 2023

CLA assistant check
All committers have signed the CLA.

Copy link

netlify bot commented Dec 12, 2023

Deploy Preview for jsonforms-examples ready!

Name Link
🔨 Latest commit 9c0780d
🔍 Latest deploy log https://app.netlify.com/sites/jsonforms-examples/deploys/659d76102882e00008ef3159
😎 Deploy Preview https://deploy-preview-2227--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.

@sdirix
Copy link
Member

sdirix commented Dec 13, 2023

Hi @SaSteffen, thanks for the contribution ❤️ We will take a look very soon

@coveralls
Copy link

coveralls commented Dec 13, 2023

Coverage Status

coverage: 84.798% (+0.003%) from 84.795%
when pulling 9c0780d on SaSteffen:enhancement/2122_add_description_to_array_renderer
into c8d3ecf on eclipsesource:master.

@sdirix sdirix force-pushed the master branch 2 times, most recently from 8bd14c2 to e9946ef Compare December 15, 2023 16:07
SaSteffen added a commit to SaSteffen/reac-jsonforms-app-demo that referenced this pull request Dec 19, 2023
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.

Thanks for the contribution! The PR looks good overall.

I have two change requests:

  • Make sure to not render the FormHelperText at all if there is no description
  • Add a test case for the existence of the description when given

@SaSteffen
Copy link
Contributor Author

@sdirix Sure. Added the conditional rendering and the two tests.

@SaSteffen SaSteffen requested a review from sdirix January 9, 2024 09:04
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.

We need to use && instead of ?? as ?? short circuits and will use the description as is and not with the FormHelperText.

Is it possible to search for the FormHelperText element in the tests too?

packages/material-renderers/src/complex/TableToolbar.tsx Outdated Show resolved Hide resolved
packages/material-renderers/src/layouts/ArrayToolbar.tsx Outdated Show resolved Hide resolved
SaSteffen and others added 2 commits January 9, 2024 11:56
Co-authored-by: Stefan Dirix <sdirix@eclipsesource.com>
Co-authored-by: Stefan Dirix <sdirix@eclipsesource.com>
@SaSteffen
Copy link
Contributor Author

We need to use && instead of ?? as ?? short circuits and will use the description as is and not with the FormHelperText.

Sorry, of course.

Is it possible to search for the FormHelperText element in the tests too?

I checked the DOM and it does not generate any element around it, even if the FormHelperText has an id attribute. If you prefer, I can put it in a div and use that one in the tests.

@sdirix
Copy link
Member

sdirix commented Jan 9, 2024

I checked the DOM and it does not generate any element around it, even if the FormHelperText has an id attribute. If you prefer, I can put it in a div and use that one in the tests.

I used nested selectors to check for the respective elements. I also added tests checking for the non-existence of them in case there is no description.

@SaSteffen
Copy link
Contributor Author

I checked the DOM and it does not generate any element around it, even if the FormHelperText has an id attribute. If you prefer, I can put it in a div and use that one in the tests.

I used nested selectors to check for the respective elements. I also added tests checking for the non-existence of them in case there is no description.

That leaves nothing else to be done, right? Thank you for the support.

@sdirix sdirix merged commit d7c15f0 into eclipsesource:master Jan 10, 2024
8 checks passed
@sdirix
Copy link
Member

sdirix commented Jan 10, 2024

Thanks for the contribution ❤️

@sdirix sdirix linked an issue Jan 30, 2024 that may be closed by this pull request
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.

Description is not rendered for array type
4 participants