Add missing material theme string widgets #1789

stanlemon · 2020-06-02T20:06:14Z

Reasons for making this change

Several string widgets are missing from the material theme, specifically color, date, date time, email and URI. Without these widgets using the various formats for fields with type 'string' will fall back to the bootstrap theme. This PR adds support for all of those widgets in material UI with the same reuse pattern as in the bootstrap theme (pass back the TextWidget with a more specific type, see https://github.com/rjsf-team/react-jsonschema-form/blob/master/packages/core/src/components/widgets/ColorWidget.js for an example).

Note that I needed to add 'type' to the WidgetProps to support this. The bootstrap widgets do not have their props typed so I believe this is why that wasn't an issue before.

Here is an example schema that you can use in the playground to see what happens when you use the material theme.

{
  "title": "String Widgets",
  "description": "String Widgets",
  "type": "object",
  "properties": {
    "date": {
      "type": "string",
      "title": "Date Widget",
      "format": "date"
    },
    "dateTime": {
      "type": "string",
      "title": "Date Time Widget",
      "format": "date-time"
    },
    "email": {
      "type": "string",
      "title": "Email Widget",
      "format": "email"
    },
    "uri": {
      "type": "string",
      "title": "URI Widget",
      "format": "uri"
    },
    "color": {
      "type": "string",
      "title": "Color Widget",
      "format": "color"
    }
  }
}

There is a similar PR just for the date widget in #1765

Checklist

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
I'm adding or updating code
- I've added and/or updated tests
- I've updated docs if needed
I'm adding a new feature
- I've updated the playground with an example use of the feature

stanlemon · 2020-06-02T20:08:04Z

I don't think this requires any documentation changes since this is just filling in gaps on the material theme, but I am happy to add anything if desired. As far as tests, I wasn't entirely sure what was best to add here. There are existing tests in StringField_test.js but they are not material-specific. Any guidance there would be appreciated.

epicfaace

Thanks for these changes -- really appreciate them. Can you add some snapshot tests that cover each widget in https://github.com/rjsf-team/react-jsonschema-form/blob/558ecd1c8866c0dba1efb2158ec792462a71aa3c/packages/material-ui/test/Form.test.tsx (meaning you would add schemas with the different string formats there)?

I'm not quite sure how to strike the balance between testing for themes -- between testing too much to be repetitive with the core library and too little to be not comprehensive enough -- for now, just settled on snapshot tests to do that (see #1688).

stanlemon · 2020-06-02T22:07:21Z

Snapshots were updated for email and url, I will add some schema for the other types.

I just noticed that my prettier extension in vscode reformatted some of the stuff I touched. I didn't find a prettier config, I did notice this comment https://react-jsonschema-form.readthedocs.io/en/latest/contributing/#coding-style and just wanted to make sure the defaults are being used and some of the formatting stuff included here is OK or if I should back it out.

stanlemon · 2020-06-02T22:12:49Z

Tests added and snapshots have been updated @epicfaace.

epicfaace

It appears that the label is not in the right place for the date (this is from the date / time playground example).

stanlemon · 2020-06-02T22:55:26Z

Interesting, looks like that happens in Chrome but not Safari. It appears to be a bug with material-ui. From what I gather I need to set this on TextWidget to make the empty render not look ugly:

InputLabelProps={{
  shrink: true
}}

Check out the example here: https://material-ui.com/components/pickers/#datepickers

This will mean that label will not bounce up above the input upon focus, which is the current default behavior on rjsf. If you're OK with that I'll work that in.

Thinking out loud here... We could use these pickers https://material-ui.com/components/pickers/ for the date and time widgets, or we could use them in the alt-date alt-datetime widgets too. Any interest in either of those options?

Here's Safari btw:

stanlemon · 2020-06-03T00:29:54Z

@epicfaace here's a set of changes that would allow for passing the shrink settings. https://github.com/stanlemon/react-jsonschema-form/pull/1/files

It also allows for further properties to be passed down to the underlying TextWidget which could come in handy. Let me know if this seems agreeable and I'll merge it into this branch.

epicfaace · 2020-06-03T01:10:37Z

Thinking out loud here... We could use these pickers https://material-ui.com/components/pickers/ for the date and time widgets, or we could use them in the alt-date alt-datetime widgets too. Any interest in either of those options?

Yeah, those would probably be ideal. We would need to add a peer dependency on @material-ui/pickers, though, so let's not do that in this PR. Let's save that for the next major version.

stanlemon · 2020-06-03T01:15:34Z

Yeah, those would probably be ideal. We would need to add a peer dependency on @material-ui/pickers, though, so let's not do that in this PR. Let's save that for the next major version.

Just to clarify: They would be ideal for the main widgets or the alt widgets?

epicfaace · 2020-06-04T02:16:31Z

Yeah, those would probably be ideal. We would need to add a peer dependency on @material-ui/pickers, though, so let's not do that in this PR. Let's save that for the next major version.

Just to clarify: They would be ideal for the main widgets or the alt widgets?

Probably for the main widgets.

epicfaace · 2020-06-04T02:17:00Z

This will mean that label will not bounce up above the input upon focus, which is the current default behavior on rjsf. If you're OK with that I'll work that in.

Sounds good to me!

stanlemon · 2020-06-04T02:19:05Z

Good deal. I merged in the change to support the shrink param, so that issue should be resolved.

stanlemon · 2020-06-04T02:58:38Z

Clean build, this one might be ready. 🤞 If you have any guidance on that last commit with regards to the typing let me know. I think it is OK, but my typescript-foo is limited so I'm not entirely confident.

epicfaace · 2020-06-07T15:07:21Z

@stanlemon can you also change the format in which the date widget gives its value?

Right now, the format doesn't seem to be valid for a "date-time" format in JSON Schema:

Compare this with the default date widget value:

…te widgets

stanlemon · 2020-06-07T16:34:14Z

@epicfaace good catch, 81b2035 should resolve that. Thanks.

epicfaace · 2020-06-07T19:10:35Z

packages/core/src/utils.js

@@ -1048,6 +1048,36 @@ export function toDateString(
  return time ? datetime : datetime.slice(0, 10);
 }

+export function utcToLocal(jsonDate) {


What exactly is the point of this function?

This is what ensures the format is correct in the Bootstrap theme. I didn’t write this, just simply relocated it to utils.

epicfaace

Looks great, thanks!

stanlemon mentioned this pull request Jun 2, 2020

#1765 - Material UI theme not showing title for date fields #1779

Closed

7 tasks

epicfaace reviewed Jun 2, 2020

View reviewed changes

stanlemon requested a review from agustin107 as a code owner June 7, 2020 16:28

stanlemon added 7 commits June 7, 2020 12:32

Add missing material UI string widgets

d7fbd94

Update snapshots for new widgets

baa8ea3

Update snapshots for additional format types

7cdb0c3

Support full parameter options for TextField and disable shrink on Da…

085db85

…te widgets

Fix types and update snapshots

435ab96

Fix formatting for datetime widget

81b2035

Update snapshots

d4a00d2

Fix test

f554351

epicfaace reviewed Jun 7, 2020

View reviewed changes

Merge branch 'master' into material-ui-string-widgets

39fb78e

epicfaace approved these changes Jun 8, 2020

View reviewed changes

epicfaace merged commit 4a05644 into rjsf-team:master Jun 8, 2020

stanlemon mentioned this pull request Jun 8, 2020

UISchema ui:placeholder only works for ui:textarea case in material-ui theme #1745

Closed

3 tasks

stanlemon mentioned this pull request Jun 8, 2020

Material UI theme not showing title for date fields #1765

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing material theme string widgets #1789

Add missing material theme string widgets #1789

stanlemon commented Jun 2, 2020

stanlemon commented Jun 2, 2020

epicfaace left a comment

stanlemon commented Jun 2, 2020

stanlemon commented Jun 2, 2020

epicfaace left a comment

stanlemon commented Jun 2, 2020 •

edited

stanlemon commented Jun 3, 2020

epicfaace commented Jun 3, 2020

stanlemon commented Jun 3, 2020

epicfaace commented Jun 4, 2020

epicfaace commented Jun 4, 2020

stanlemon commented Jun 4, 2020

stanlemon commented Jun 4, 2020

epicfaace commented Jun 7, 2020

stanlemon commented Jun 7, 2020

epicfaace Jun 7, 2020

stanlemon Jun 7, 2020

epicfaace left a comment

Add missing material theme string widgets #1789

Add missing material theme string widgets #1789

Conversation

stanlemon commented Jun 2, 2020

Reasons for making this change

Checklist

stanlemon commented Jun 2, 2020

epicfaace left a comment

Choose a reason for hiding this comment

stanlemon commented Jun 2, 2020

stanlemon commented Jun 2, 2020

epicfaace left a comment

Choose a reason for hiding this comment

stanlemon commented Jun 2, 2020 • edited

stanlemon commented Jun 3, 2020

epicfaace commented Jun 3, 2020

stanlemon commented Jun 3, 2020

epicfaace commented Jun 4, 2020

epicfaace commented Jun 4, 2020

stanlemon commented Jun 4, 2020

stanlemon commented Jun 4, 2020

epicfaace commented Jun 7, 2020

stanlemon commented Jun 7, 2020

epicfaace Jun 7, 2020

Choose a reason for hiding this comment

stanlemon Jun 7, 2020

Choose a reason for hiding this comment

epicfaace left a comment

Choose a reason for hiding this comment

stanlemon commented Jun 2, 2020 •

edited