Skip to content

Enhancement/2122 add description to array renderer - #2227

Merged
sdirix merged 7 commits into
eclipsesource:masterfrom
SaSteffen:enhancement/2122_add_description_to_array_renderer
Jan 10, 2024
Merged

Enhancement/2122 add description to array renderer#2227
sdirix merged 7 commits into
eclipsesource:masterfrom
SaSteffen:enhancement/2122_add_description_to_array_renderer

Conversation

@SaSteffen

Copy link
Copy Markdown
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

CLAassistant commented Dec 12, 2023

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@netlify

netlify Bot commented Dec 12, 2023

Copy link
Copy Markdown

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

sdirix commented Dec 13, 2023

Copy link
Copy Markdown
Member

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

@coveralls

coveralls commented Dec 13, 2023

Copy link
Copy Markdown

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

@sdirix sdirix left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

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

@SaSteffen
SaSteffen requested a review from sdirix January 9, 2024 09:04

@sdirix sdirix left a comment

Copy link
Copy Markdown
Member

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?

Comment thread packages/material-renderers/src/complex/TableToolbar.tsx Outdated
Comment thread packages/material-renderers/src/layouts/ArrayToolbar.tsx Outdated
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
Copy Markdown
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

sdirix commented Jan 9, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
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
@sdirix

sdirix commented Jan 10, 2024

Copy link
Copy Markdown
Member

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