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

Make material renderers aware of input variant in theme #2182

Conversation

sebastianfrey
Copy link
Contributor

@sebastianfrey sebastianfrey commented Sep 29, 2023

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

netlify bot commented Sep 29, 2023

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

@lucas-koehler lucas-koehler left a comment

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?

packages/material-renderers/src/util/theme.ts Outdated Show resolved Hide resolved
packages/material-renderers/src/util/theme.ts Outdated Show resolved Hide resolved
@sebastianfrey
Copy link
Contributor Author

sebastianfrey commented Oct 5, 2023

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.

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

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.

@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
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

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.

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

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
6 checks passed
@sebastianfrey
Copy link
Contributor Author

Hi @lucas-koehler,

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

@lucas-koehler
Copy link
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
Contributor Author

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

@lucas-koehler
Copy link
Contributor

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

@sebastianfrey
Copy link
Contributor Author

@lucas-koehler Thank you very much!

@MikeyZat
Copy link

@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
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