-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add ZoneGrid component #3512
Add ZoneGrid component #3512
Conversation
+ Cleanup headerName extraction
+ Move MultiZoneGrid to mobile
this.persistenceModel?.clear(); | ||
|
||
if (this.restoreDefaultsFn) { | ||
await this.restoreDefaultsFn(); |
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.
use conditional eval on 183 to match line 180?
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 tried that, but IJ complained about it with the await
}); | ||
|
||
// 2) Ensure mapping respects configured limits | ||
const limit = this.limits?.[zone]; |
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 wonder about 'constraints' vs. 'limits'
mobile/cmp/multiZoneGrid/Types.ts
Outdated
/** Max number of fields that should be mapped to the zone */ | ||
max?: number; | ||
/** Array of allowed fields for the zone */ | ||
only?: string[]; |
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.
allowedFields
?
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.
Think its a better name, and happy to change, but wanted to point out that this name (and 'limits') were taken from the Sencha API.
…, rather than left / right column + Add sort control to MultiZoneMapper
+ Add desktop ZoneMapper, with corresponding button and context menu action
…t / right column during Store field creation
+ Remove default `restoreDefaultsWarning`
+ Make mobile zone mapper true fullscreen
onInteraction: (willOpen, e?) => { | ||
if (isOpen && !willOpen) { | ||
// Prevent clicks with Select controls from closing popover | ||
const selectPortal = document.getElementById(MENU_PORTAL_ID), |
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.
yikes -- this could hit any application. Is there a way we can change select to not leak these?
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, we have similar patterns in some client apps that would be great to unwind in favour of built-in protection. I'll file a ticket 👍
See Toolbox PR: xh/toolbox#679
Hoist P/R Checklist
Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.
develop
branch as of last change.breaking-change
label + CHANGELOG if so.If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.