-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Complex forms and components UI #1121
base: develop
Are you sure you want to change the base?
Conversation
UI unit Tests12 tests 12 ✅ 0s ⏱️ Results for commit 679571d. ♻️ This comment has been updated with latest results. |
…ead pass them directly to where they need to go skipping the event handler
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 made some changes as I went, feel free to adjust any of them as needed. I left 2 questions/comments.
<button slot="trigger" class="flex-1 flex justify-between items-center text-left max-w-full overflow-hidden" | ||
on:click={() => { | ||
if (disableExpand && !disabledEntry) { | ||
// the change event above is not in use, so we need to do it here |
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.
is this comment still correct? if I remove on:change
from the Expansion panel then the sections don't get highlighted correctly
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.
The comment is valid in the context of the if
it's inside.
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.
Actually, only this part: if (disableExpand)
.
The disabledEntry
part should probably just set the disabled attribute of the button it's on.
<Button disabled on:click={() => openPicker = true} icon={mdiPlus} variant="fill-light" color="success" size="sm"> | ||
<div class="max-sm:hidden">Add Complex Form Type</div> | ||
</Button> | ||
<!-- TODO: implement form type picker or dropdown? <EntryOrSensePicker title="Add complex form" bind:open={openPicker} noSenses on:pick={(e) => addComplexFormType(e.detail)} /> --> |
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.
looks like this was forgotten. Still need to have a pick for the form type
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, I'd like to do that in a follow up PR.
Resolves most of #1089