Skip to content

Make material renderers aware of input variant in theme - #2182

Merged
lucas-koehler merged 7 commits into
eclipsesource:masterfrom
sebastianfrey:1797-make-material-renderes-aware-of-input-variant
Oct 9, 2023
Merged

Make material renderers aware of input variant in theme#2182
lucas-koehler merged 7 commits into
eclipsesource:masterfrom
sebastianfrey:1797-make-material-renderes-aware-of-input-variant

Conversation

@sebastianfrey

@sebastianfrey sebastianfrey commented Sep 29, 2023

Copy link
Copy Markdown
Contributor

Resolves #1797.

This PR makes material renderers aware of the input variant defined in the global MUI theme.

The example app was enhanced with a select, which allows to change the input variant.

@netlify

netlify Bot commented Sep 29, 2023

Copy link
Copy Markdown

Deploy Preview for jsonforms-examples ready!

Name Link
🔨 Latest commit 67cf64e
🔍 Latest deploy log https://app.netlify.com/sites/jsonforms-examples/deploys/652039776e25f00008ba922d
😎 Deploy Preview https://deploy-preview-2182--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.

@sebastianfrey
sebastianfrey force-pushed the 1797-make-material-renderes-aware-of-input-variant branch from 0c5f106 to 38fc730 Compare September 29, 2023 20:38
@lucas-koehler
lucas-koehler self-requested a review October 4, 2023 13:19

@lucas-koehler lucas-koehler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @sebastianfrey
Thank you very much for the contribution ❤️
The changes already look pretty good to me and worked well in the example app!
Besides the small remarks inline, I have one question:

Could we make handing in InputComponent to the controls optional and fall back to material's Input? With this, we would not break custom renderers that use these controls. Do you think that makes sense?

Comment thread packages/material-renderers/src/util/theme.ts Outdated
Comment thread packages/material-renderers/src/util/theme.ts Outdated
Comment thread packages/material-renderers/src/mui-controls/MuiInputInteger.tsx Outdated
Comment thread packages/material-renderers/src/mui-controls/MuiInputNumber.tsx Outdated
@sebastianfrey

sebastianfrey commented Oct 5, 2023

Copy link
Copy Markdown
Contributor Author

Hi @lucas-koehler,

  • Input control implementations now determine their InputComponent separately. This should probably avoid breaking custom renderer implementations.
  • Props from useInputComponent() were removed. Passing the main components props to useThemeProps() from MUI has now advantage in this case.
  • interface is now used in favor of type declaration.

Let me know if anything else is missing.

@lucas-koehler lucas-koehler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @sebastianfrey
Thank you for the updates! I have just one remaining small comment inline.

Comment thread packages/material-renderers/src/controls/MaterialInputControl.tsx Outdated
@sebastianfrey
sebastianfrey force-pushed the 1797-make-material-renderes-aware-of-input-variant branch from 1ad814e to 67cf64e Compare October 6, 2023 16:44
@sebastianfrey

Copy link
Copy Markdown
Contributor Author

Hi @lucas-koehler,

Your requested change should be fixed now.

After browsing through the example forms, I have noticed that MuiSelect inputs were not displayed correctly. This should be fixed now. Also, within MuiSelect an unnecessary Omit was removed.

@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 84.469%. remained the same when pulling 67cf64e on sebastianfrey:1797-make-material-renderes-aware-of-input-variant into 694020b on eclipsesource:master.

@lucas-koehler lucas-koehler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM now. Thanks again for the contribution and the updates :)

@lucas-koehler
lucas-koehler merged commit a6fa3c8 into eclipsesource:master Oct 9, 2023
@sebastianfrey

Copy link
Copy Markdown
Contributor Author

Hi @lucas-koehler,

Is there the possibility to publish a pre-release version for this PR?

@lucas-koehler

Copy link
Copy Markdown
Contributor

Hi @sebastianfrey ,
I cannot do this at the moment but I'll bring it up with the team later this week. I think that should be possible :)

@sebastianfrey

Copy link
Copy Markdown
Contributor Author

@lucas-koehler Ok thanks, no rush here please!

@lucas-koehler

Copy link
Copy Markdown
Contributor

Hi @sebastianfrey , we released 3.2.0-alpha.0 just now :)

@sebastianfrey

Copy link
Copy Markdown
Contributor Author

@lucas-koehler Thank you very much!

@MikeyZat

Copy link
Copy Markdown

@lucas-koehler do you have an ETA for the 3.2.0 release of JSON forms? We are really excited about this particular change being published in a stable release 🎉

@lucas-koehler

Copy link
Copy Markdown
Contributor

Hi @MikeyZat , our current plan is to publish a 3.2.0-beta.0 version before Christmas and then do the 3.2.0 release in January.

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.

Upgrade Material Renderer Inputs to be aware of Variant

4 participants