feat: implement first draft summary overview page #1713
Conversation
There was a problem hiding this comment.
Thanks for taking a go at this. It's a good start, but I think we can simplify this a lot. Most of my comments are namings but some are quite "fundamental. Would love for you to have a look. :)
Disclaimer: I did not have enough time to look at most of the tests, the FormFlowPage and the ummaryOverviewBoxArray yet.
| .object({ | ||
| title: StrapiStringOptionalSchema, | ||
| stepId: z.string(), | ||
| boxItems: z |
There was a problem hiding this comment.
Not a big fan of the wording "box" and "boxItems" I feel that's not really what it is? Isn't this about pages and fields or similar?
There was a problem hiding this comment.
right now, we don't add only one page, but one or many pages, depends what the content creator would like to see and it'll render basically a box as the image below
There was a problem hiding this comment.
The example that you posted here is also just one page, right? And it should always just be one page because you can only link back to one page. so I think pages actually is the perfect word here, esp. to discipline ourselves to use the component correctly.
There was a problem hiding this comment.
True, for this one it's one page, but the image below it has three pages. It's an edge case wished by the designers. I can change to be pages and pagesItems in Strapi, but it won't be very realistic what the content creator could do in Strapi, for this reason I decided for this generic name box.
There was a problem hiding this comment.
And if you press on "Bearbeiten" you only see the first page or what is the expected behaviour? I would encourage you to challenge that with your designers. It is a "weird" pattern that must also feel weird from a user perspective. Did you challenge that already?
There was a problem hiding this comment.
I have already challenged and they accepted to have two boxes. But due the problems for the array summary, this option now it's available to have two pages in one box. So the content creators can do it now. About the button, it'll go to the first page available for the flow.
The content creators have the option to have even two boxes for two pages or one box for two or more pages (depends how it's the flow it's configured)
There was a problem hiding this comment.
phew...I see..that will probably get very messy in the future, I imagine. But I see your point. Maybe we can also address this in the meeting tomorrow.
| const itemValue = getItemValueBox( | ||
| translations, | ||
| userData, | ||
| field, | ||
| displayEmptyValue, | ||
| ); | ||
|
|
||
| const inlineItemValues = getInlineItemValues( | ||
| userData, | ||
| translations, | ||
| inlineItems, | ||
| ); |
There was a problem hiding this comment.
Why do you make a differentiation between the two? Can't we just always use inlineItems as they use ValueBox under the hood anyways?
There was a problem hiding this comment.
nope, we use the inlineItems only for the edge cases when I need to have two or more fields in the same line, otherwise it should use the itemValue
There was a problem hiding this comment.
I understand that and am convinced that we need both possibilities. I'm just questioning the how. Why do we need two separate fields in Strapi for that? inlineItems in itself would already cover all cases - normal and edge. So we can spare an additional field in Strapi.
There was a problem hiding this comment.
You are right, but the main idea of the inlineItems is to cover the edge cases when we need two or more fields values in one line. Most of the time we only need one field in the Summary, where the inlineItems makes not needed. I'll address this question today meetings and ask designers opinion about it.
There was a problem hiding this comment.
Hi! Did you ask about this yesterday, too when I was gone? I still think we're having two places that do the same thing where one is just the edge case of the other. And I even think of it the other way around than you, I think:
fieldasks for one field name.inlineItemsasks for multiple field names.- otherwise, they are completely the same.
- instead of having either one item or a list of the same items, I propose we have a list of those items that just most times only has one element.
We would spare ourselves the whole edge-case handling if we just include it in the default case.
We can then also move the displayEmptyValue within the array inlineItems, even closer and more obviously related to the field field. That way you could even display defaultValues inline if you'd want to. This is just a cleaner API/design and gives more flexibility.
|
Ticket: https://app.asana.com/0/1207022303033556/1208899940491471