DataViews: Experiment with field types and inline editing #56945
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
added
[Type] Experimental
ntsekouras
requested review from
mcsf,
youknowriad,
oandregal and
jorgefilipecosta
|
Size Change: +743 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 55c6a2c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7167738917
|
oandregal
reviewed
Dec 11, 2023
| @@ -257,11 +265,25 @@ export default function PagePages() { | |||
| { | |||
| header: __( 'Status' ), | |||
| id: 'status', | |||
| getValue: ( { item } ) => | |||
| fieldType: ENUMERATION_TYPE, | |||
There was a problem hiding this comment.
This field already has the type: ENUMERATION_TYPE. What is it fieldType for?
There was a problem hiding this comment.
I've added a fieldType prop, but probably should be merged with the current type prop used for filters
I didn't want to mess with the filters for now in case different types('date, filter, etc..`) broke something.
There was a problem hiding this comment.
If there's any, I'd rather bring them up sooner than later.
There was a problem hiding this comment.
Agree with this, I feel like type and fieldType are the same thing and we should just have one "type".
There was a problem hiding this comment.
type and fieldType should just be the same thing. 👍
oandregal
reviewed
Dec 11, 2023
| STATUSES.find( ( { value } ) => value === item.status ) | ||
| ?.label ?? item.status, | ||
| type: ENUMERATION_TYPE, | ||
| elements: STATUSES, | ||
| editElements: STATUSES.filter( ( { value } ) => |
There was a problem hiding this comment.
What do we need editElements for? I'd think the user should be able to select any of the elements' values.
There was a problem hiding this comment.
Some enumeration fields can have different available options between filtering and editing. An example is the status field where some statuses would require more actions beside updating a single value - they could require from the user to set more values(future status needs also a date input).
There was a problem hiding this comment.
My point is: I don't see editElements as a target API we need. Instead, I feel we should introduce the edit function for fields that cannot use the default edit.
We don't need to do it all at once, of course. We can have a editable: false or something like that for fields that don't allow inline editing yet.
oandregal
reviewed
Dec 11, 2023
| @@ -212,7 +213,14 @@ export default function PagePages() { | |||
| { | |||
| header: __( 'Title' ), | |||
| id: 'title', | |||
| fieldType: 'string', | |||
There was a problem hiding this comment.
Unlike the enumeration type, string would be a free entry field. Do we need some sort of basic validation and the ability to add more? I'm thinking about things such as the length of the title doesn't exceed what a given API supports, whether it needs to adhere to a certain format even if it's a string, etc.
There was a problem hiding this comment.
Probably this should be baked in the edit function of each field. For your example though that would mean having the validation separately. We can see how it goes when we try this for more fields.
There was a problem hiding this comment.
This was my thinking: do we delegate validation to the edit function (the default provided by the field type or the custom one provided by the field)? or do we need a schema/validation mechanism that works independently from the edit?
For example, what do we do with an email field? Is it of type string or email? If the consumer provides a custom edit function, checking that the string introduced is an email would be their responsibility. However, if we use a proper validation/schema API, it's dataviews' – even if it's using a custom edit function.
I feel we don't have enough field variety to tell yet, though that's a question I wanted to bring up for us to keep in mind.
There was a problem hiding this comment.
I fell down a rabbit hole thinking about this conversation. :)
I think it could help to think about the model baked into HTML forms:
- There are fundamental types, like
text,file,checkbox, etc. - There are some basic specialisations for text, like
number,email,password - There are some — but few — type hints like
inputmode="tel|numeric|..."(ref) - Everything else is validation facilitated by attributes:
<input required><input minlength="3" pattern="[A-z]+">- with an escape hatch for more custom validation in the form of
input.setCustomValidity
I put up a quick example here: https://jsfiddle.net/mcsf/Lqjsebgt/1/.
(Basically, browsers do a ton of validation out of the box, and when you need to go beyond static attributes you can work with the browser to add special cases. This point isn't probably too relevant for Gutenberg, but still a cool thing to observe.)
How does this apply to our fields? Well, field have a certain type, but — if we were to adopt this HTML model — validation is a separate concern.
➡ Relevant read: the ValidityState interface
|
For templates, there's an action (rename) that should be inline editing and that provides an interesting use case: even if the field is editable, perhaps not all the rows are. This is: Gravacao.do.ecra.2023-12-14.as.14.18.05.mov |
mcsf
reviewed
Dec 19, 2023
Comment on lines
+28
to
+32
| icon={ | ||
| element.value === currentValue ? check : undefined | ||
| } | ||
| style={ { | ||
| backgroundColor: element.color || undefined, |
There was a problem hiding this comment.
Minor note, but || undefined is redundant, no?
| @@ -257,11 +265,25 @@ export default function PagePages() { | |||
| { | |||
| header: __( 'Status' ), | |||
| id: 'status', | |||
| getValue: ( { item } ) => | |||
| fieldType: ENUMERATION_TYPE, | |||
There was a problem hiding this comment.
type and fieldType should just be the same thing. 👍
| @@ -212,7 +213,14 @@ export default function PagePages() { | |||
| { | |||
| header: __( 'Title' ), | |||
| id: 'title', | |||
| fieldType: 'string', | |||
There was a problem hiding this comment.
I fell down a rabbit hole thinking about this conversation. :)
I think it could help to think about the model baked into HTML forms:
- There are fundamental types, like
text,file,checkbox, etc. - There are some basic specialisations for text, like
number,email,password - There are some — but few — type hints like
inputmode="tel|numeric|..."(ref) - Everything else is validation facilitated by attributes:
<input required><input minlength="3" pattern="[A-z]+">- with an escape hatch for more custom validation in the form of
input.setCustomValidity
I put up a quick example here: https://jsfiddle.net/mcsf/Lqjsebgt/1/.
(Basically, browsers do a ton of validation out of the box, and when you need to go beyond static attributes you can work with the browser to add special cases. This point isn't probably too relevant for Gutenberg, but still a cool thing to observe.)
How does this apply to our fields? Well, field have a certain type, but — if we were to adopt this HTML model — validation is a separate concern.
➡ Relevant read: the ValidityState interface
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Related: #55083, #55101
This is a quick and rough exploration of using
field typesin DataViews to have default render functions, editable fields and also this should expand to the creation of filters. It's very rough right now to figure out some constraints and discuss other approaches.This PR is not about the design at first(it's mine and it's bad 😆 ), but mostly to explore the needed API.
Notes
editableunder specific conditions so we might need anisEligibleToEditcallback or something.fieldTypeprop, but probably should be merged with the currenttypeprop used for filtersenumerationfields can have different available options between filtering and editing. An example is thestatusfield where some statuses would require more actions beside updating a single value - they could require from the user to set more values(futurestatus needs also a date input).getValue. I think it should be about getting the raw data that an item is actually storing. The thing I changes here was forstatuswhere ingetValuewe would also return alabel, but it seems it's therender's function responsibility. This could make sense to field types with a relation like theauthorwhere the entities persist the user id.Screen.Recording.2023-12-11.at.3.01.09.PM.mov