-
Notifications
You must be signed in to change notification settings - Fork 419
refactor: Improve spacing behavior for nested layouts with conditional visibility #2533
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
base: master
Are you sure you want to change the base?
Conversation
…p layouts now do not have extra paddings
…add up, also fixed the issue where some of the children can be hidden for which no padding is added
…remove extra padding
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@sdirix @lucas-koehler please review - new demo example employee-registration |
|
found an issue with horizontal layout when we do not use padding but gap in order to fix the issue with the elements that are invisible because of the security to not have extra space but this breaks when we want to use number of columns in the UI model like |
…ut can't be easily fixed with gap and allowing the v-col cols property to work
|
fixing the horizontal and vertical layout by using no-gutters for vertical and fluid for both horizontal and vertical layouts |
…nvisible elements to not have extra padding from the v-col
…ements that are not visible.
|
fix removing extra paddings for invisible elements by not creating the v-col for such elements. |
…not have v-container paddings
|
@sdirix @lucas-koehler please review - the pr is ready now |
sdirix
left a comment
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.
Thanks for the contribution ❤️
Again this is difficult to review. This PR has 6+ different concerns wrapped into one pull request. Separate small PRs would make reviewing easier and would reduce the wait time for you to get feedback.
This PR addresses:
- Layout visibility filtering (main goal?)
- Converting v-card to v-expansion-panels
- Spacing/padding cleanups
- Unrelated control fixes
- Dev changes
- Unrelated Vue package changes, not directly tied to Vue-Vuetify
There are so many functionality and layout changes that it makes it really hard to test all of them besides a shallow "looks fine to me".
Please split them in the future.
Functionality wise most changes look fine.
The v-expansion-panels refactoring is an obvious breaking change for all adopters using them, in behavior as well as in configuration (e.g. when they used 'v-card' props to customize them). So this should be mentioned in the Breaking Changes / Migration guide. Again this would be easier as a separate PR.
Overall I don't have confidence for all the layout changes, I might have overlooked something.
Please have a look at the comments.
| }, | ||
| }, | ||
| methods: { | ||
| isVisible(uischema: UISchemaElement, path: string): boolean { |
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.
This isVisible method is copy-pasted across 3 files: HorizontalLayoutRenderer, VerticalLayoutRenderer, GroupRenderer. Maybe this should be a shareable composition? Maybe the same for visibleElementsWithIndex? This wouild also allow to unify the behavior between Horizontal and Vertical rendering as they implement this logic differently now.
| @@ -1,5 +1,5 @@ | |||
| <template> | |||
| <div>No applicable renderer found.</div> | |||
| <div :style="{ color: 'red' }">No applicable renderer found.</div> | |||
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 usually avoid hard coded styling in the non-renderer and Vanilla packages. This should get a proper class so it can be styled as required by adopters.
I know we do it in React too but this is more of an old historical oversight.
I would prefer using a class over inline styling.
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.
This change is unnecessary
| > | ||
| <v-col | ||
| cols="12" | ||
| v-if="isVisible(element, layout.path)" |
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.
Instead of this v-if we could already iterate over the filtered list like in the other renderers
| return { ...useVuetifyLayout(useJsonFormsLayout(props)), jsonforms }; | ||
| }, | ||
| methods: { | ||
| // helper method to check visibility and if not visible to remove the v-col since otherwise it will add extra padding |
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.
Not sure the comment is needed here
| <v-expansion-panel-title | ||
| :class="styles.group.label" | ||
| v-bind="vuetifyProps('v-expansion-panel-title')" | ||
| >{{ layout.label }}</v-expansion-panel-title |
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.
Now that Groups are clickable it's a bit weird that the label can be empty. However I also don't have a good suggestion for this case. Maybe add an icon if the label is empty? Or revert to the old v-card grouping? Not sure. See the rule example to check how this looks like in the UI.
visible: falseMake the group layout and array layouts/control to use collapsable panels to make it easy to deal with array with large number of elements if we want to collapse the whole array UI into just one row.
Make the space usage similar to react - e.g. remove exta margins and/or paddings
Other minor fixes