describo / crate-builder-component

A VueJS UI component to build an RO-Crate
MIT License
6 stars 3 forks source link

Profile input constraints #89

Closed beepsoft closed 5 months ago

beepsoft commented 5 months ago

This implements the feature described in https://github.com/describo/crate-builder-component/issues/81, thanks to @csontosreka

To test is:

npm run develop

Use "blank" crate and select "A profile with constraints". There you will have fields with all combinations of possible constraints.

marcolarosa commented 5 months ago

Looks good but I'm wondering why the date / time granularity code has been overloaded into the Text primitive?

Have a look at https://element-plus.org/en-US/component/date-picker.html#date-formats - can we move all of the date time handling code to the date / datetime primitives? It seems that this would simplify things as well - maybe granularity can be replaced with a format string that is passed directly to the component itself. Then a user could set YYYY-MM or MM-YYYY; really whatever the profile author wants.

It was confusing to me to see type: "Date" in the profile but then find the code in type: Text

marcolarosa commented 5 months ago

oh - and thankyou very much @csontosreka !

beepsoft commented 5 months ago

Looks good but I'm wondering why the date / time granularity code has been overloaded into the Text primitive? ... It was confusing to me to see type: "Date" in the profile but then find the code in type: Text

So, you suggest that we move all Date/DateTime logic, even if we display a text field from Text.component.vue to Date.component.vue/DateTime.component.vue?

Or, do you suggest to use el-date-picker in more scenarios? Like, it can also be used to pick year-month, year, etc. We actually decided to use a simple text input instead because the el-date-picker is not really flexible or inconvenient when it comes to dates before say the 20th century, eg. a date like 899. You need to click a lot to get there. On the otherhand, if you try to enter it by hand, it won't accept "899" as input only "0899" but then it displays it as "899".

Also we want to allow multiple granularity to be entered for the same field. Eg. YYYY or YYYY-MM. This cannot be done with el-date-picker.

Regarding using format strings instead of these preset granularity values, we can do that. We just followed the original Date input format here, which is constant YYYY-MM-DD. If we switch to format strings, should this be applicable to the default Date type (currently YYYY-MM-DD) as well?

marcolarosa commented 5 months ago

Looks good but I'm wondering why the date / time granularity code has been overloaded into the Text primitive? ... It was confusing to me to see type: "Date" in the profile but then find the code in type: Text

So, you suggest that we move all Date/DateTime logic, even if we display a text field from Text.component.vue to Date.component.vue/DateTime.component.vue?

Or, do you suggest to use el-date-picker in more scenarios? Like, it can also be used to pick year-month, year, etc. We actually decided to use a simple text input instead because the el-date-picker is not really flexible or inconvenient when it comes to dates before say the 20th century, eg. a date like 899. You need to click a lot to get there. On the otherhand, if you try to enter it by hand, it won't accept "899" as input only "0899" but then it displays it as "899".

I understand. I was thinking about using the date primitive for all the date scenario's with a format but I didn't consider the issue of manually adding in odd dates like your example 899.

Also we want to allow multiple granularity to be entered for the same field. Eg. YYYY or YYYY-MM. This cannot be done with el-date-picker.

Ok.

Regarding using format strings instead of these preset granularity values, we can do that. We just followed the original Date input format here, which is constant YYYY-MM-DD.

I think it's a bit confusing in the code to have a date / datetime type in the profile but have the text component loaded. These formats are just an extension of the valid data types for a text element so what if it was just type = Text in the profile but matching a format like [ YYYY-MM, YYYY, ....] or whatever. And as I'm writing this, a regex on the text element could actually just do this which would remove all of the granularity code...

So Text with a YYYY regex would look like:

{
    "id": "https://schema.org/text",
     ...
    "regex": "/^-?\d{1,4}$/",
    "type": ["Text"]
},

or with a format key

{
    "id": "https://schema.org/text",
    ...
    "format": [ 'YYYY-MM', 'YYYY' ],
    "type": ["Text"]
},

I think I like having the format key in addition to the regex as format is an easier way to specify a set of date formats. In this mode, if format is defined then it is assumed to only mean a date format and would be treated that way. Anything that doesn't look like a date format should just be ignored (e.g putting a regex into the format array: format = [ '^a', 'YYYY'] - the regex is just ignored).

What do you think? Can that work and does it simplify the Text primitive?

beepsoft commented 5 months ago

Ok. So we would just use text and allow the use of either format/regex.

But instead of format should we maybe use dateFormat? The reason is that currently as we use "dateGranularity" with Date, we know that the input must be valid as a date. For example, when dateGranularity is "month", ie. we expect YYYY-MM and we expect that MM is between 1-12. The same with dateGranularity=day. In this case accept 2024-02-29 but not 2025-02-29 (no leap day in 2025).

So, it might be a good idea to make it explicit that when you define a dateFormat the input will be validated both regarding that the inputs are valid number, but also that they are valid as date values.

marcolarosa commented 5 months ago

Excellent - thankyou @beepsoft

csontosreka commented 5 months ago

I have implemented the fix based on the above suggestions. Could you take a look at the branch named profile-input-constraints?

marcolarosa commented 5 months ago

@csontosreka Great work! Merging now.

marcolarosa commented 5 months ago

@csontosreka I'm moving the documentation you wrote in the readme to https://describo.github.io/documentation/profiles/types.html (if you need to change it submit a pull request at https://github.com/describo/documentation/blob/master/src/profiles/types.md)