Skip to content
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

feat(ui5-flexible-column-layout): public layoutsConfiguration property #9290

Closed
wants to merge 9 commits into from

Conversation

kineticjs
Copy link
Contributor

Add public layoutsConfiguration property to allow the app customize the column proportions per screen size and layout.
Also, update the layoutsConfiguration fields whenever the user interactively changes the column proportions by dragging the columns separator.

@kineticjs kineticjs marked this pull request as ready for review June 27, 2024 07:44
@dobrinyonkov
Copy link
Contributor

As synced offline, the layoutsConfiguration is now a public property that is being changed by the component upon user interaction, it would help to fire an event and notify the application about those changes.

Copy link
Contributor

@dobrinyonkov dobrinyonkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, it works and looks geat. I've only commented on two concerns of mine that we can discuss offline, if needed.

Also, could we add a test for the layout-configuration-change event?

return false;
}

const hasColumnBelowMinWidth = pxWidths.some(pxWidth => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure whether this could cause an issue but it seems that this check returns false whenever we have less than 3 columns because of their 0 width.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this issue had popped up earlier, this is why the check for width also contained pxWidth > 0

if (this.layout !== newLayout) {
this.layout = newLayout;
this.fireLayoutChange(true, false);
}
if (oldColumnLayout !== newColumnLayout) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those two seems to be arrays, comparing them with "===" will compare them by reference and would result in false. Unless this is intentional, we may need to deep check if the fields have changed.

Copy link
Contributor Author

@kineticjs kineticjs Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, corrected now to compare the actual content

@github-actions github-actions bot added the Stale label Aug 21, 2024
@github-actions github-actions bot closed this Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants