-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Expand nested components #20453
Expand nested components #20453
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial feedback, overall i find the functions confusing and it's unclear why you've done what you've done at the moment, but I think thats a lack of explanation rather than a criticism of your solution :)
packages/core/content-type-builder/admin/src/utils/getMaxDepth.ts
Outdated
Show resolved
Hide resolved
...ilder/admin/src/components/DataManagerProvider/utils/retrieveComponentsThatHaveComponents.ts
Outdated
Show resolved
Hide resolved
...tent-type-builder/admin/src/components/DataManagerProvider/utils/retrieveNestedComponents.ts
Outdated
Show resolved
Hide resolved
...uilder/admin/src/components/DataManagerProvider/utils/tests/retrieveNestedComponents.test.ts
Outdated
Show resolved
Hide resolved
...ges/core/content-type-builder/admin/src/components/FormModal/utils/getAttributesToDisplay.ts
Show resolved
Hide resolved
...tent-type-builder/admin/src/components/DataManagerProvider/utils/retrieveNestedComponents.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not QAd but the code looks much better.
packages/core/content-type-builder/admin/src/utils/getMaxDepth.ts
Outdated
Show resolved
Hide resolved
packages/core/content-type-builder/admin/src/utils/getMaxDepth.ts
Outdated
Show resolved
Hide resolved
packages/core/content-type-builder/admin/src/utils/tests/getMaxDepth.test.ts
Show resolved
Hide resolved
...tent-type-builder/admin/src/components/DataManagerProvider/utils/retrieveNestedComponents.ts
Outdated
Show resolved
Hide resolved
...tent-type-builder/admin/src/components/DataManagerProvider/utils/retrieveNestedComponents.ts
Outdated
Show resolved
Hide resolved
const maxDepth = getChildrenMaxDepth(uid, componentsThatHaveOtherComponentInTheirAttributes); | ||
const componentDepth = getComponentDepth(targetUid, nestedComponents); | ||
const totalDepth = maxDepth + componentDepth; | ||
return totalDepth <= MAX_COMPONENT_DEPTH; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just wondering if we should provide feedback to the user about why
components have been excluded from the list?
I guess we had the same lack of feedback problem before but it was probably more
obvious what was going on as any nesting was filtered out (is that right?)
Now the max depth is set at 6 it could be harder to work out why the filtering
is happening. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah some form of feedback would've been nice,
however it's unclear to me how we could give feedback either, the current way things look
there isn't much space or place to display useful, info, I mean maybe I could put a simple general message such as components that would exceed the nested limit of 6 are excluded from the list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's a bit tricky maybe we should run this by Marco and track it as an improvement after this is merged ?
It looks like there is a bug in the CTB where I'm unable to scroll on the list of components when adding one to another. Do you see this too? |
Sorry for the long comment. I'm seeing some strange behaviour in the CTB. I've let up Level 1 - Level 6 components (level 1 has level 2 nested, 2 has 3 etc etc). I was able to set this up by creating all levels with no component fields and then starting from Level 1 I added Level 2 and on and on. My test was going to be to try and add Level 1 to another component and see that it is excluded from the list (as we've now exceeded 6 level, does that make sense?) Instead I'm able to see all levels in the component drop down but when I add any I get an error from the backend like
|
well interesting, well in terms of being viewable that's accurate cause that's how i defined level 6, you'd have to check the parent of level 1 and see, anyways the server error is strange to me I didn't face it, perhaps it's the method things have been added, I wasn't aware of any server limitations on nesting components |
yep seems to be new didn't exist before I merge |
}) | ||
.required(), | ||
// TODO: Add correct server validation for nested components | ||
component: yup.string().required(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you had plans to do this but I think if we're saying that the max level of nesting is 6 that should be enforced on the frontend and backend. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we actually never planned to enforce on the backend, and whatever was previously here was not working from all places anyways, so yeah would leave it to a different point in time since this will just add more to the complexity and no one should be hitting this point anyways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think the validation here should match our intentions for the feature
and prevent incorrect setups being made through the backend. Could we somehow
count the deepest level the component is being used at in a util & use it
similarly to hasComponent
?
wdyt? @innerdvations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would it take to add such validation, is it only about checking the number of nested components in the received data?
Anyway, we should try to have the same rules in the admin and server. So I would add this check here too if it's possible and doesn't deviate from the original PR goal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do that at a second iteration, we would be adding the limitation rules to the server as well
but the goal here was to add the limitations only on the frontend considering the server already accepted any amount of nested components, and too also move with quickly.
we would basically have to duplicate the same depth counting algorithm on the frontend to add the rule, it's a little complex but should get the job done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA'd and everything is working 👍🏻
I do think we should track this in JIRA and add some more specific BE validation for the components #20453 (comment)
What does it do?
Why is it needed?
How to test it?