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: checkbox for inversion of visibility value #512

Conversation

FabienArcellier
Copy link
Collaborator

@FabienArcellier FabienArcellier commented Aug 10, 2024

implement #354

This PR implement the reverse option that allow a developer to reverse the visibility logic. It also implements binding persistence when the user returns to Yes

Peek 2024-08-10 07-10

@FabienArcellier FabienArcellier marked this pull request as ready for review August 10, 2024 05:11
@FabienArcellier FabienArcellier added the enhancement New feature or request label Aug 10, 2024
@FabienArcellier FabienArcellier self-assigned this Aug 10, 2024
Comment on lines 58 to 80
@input="
(ev: Event) =>
setVisibleValue(
component.id,
component.visible,
(ev.target as HTMLInputElement).checked,
)
"
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a shorter syntax

Suggested change
@input="
(ev: Event) =>
setVisibleValue(
component.id,
component.visible,
(ev.target as HTMLInputElement).checked,
)
"
@input="setVisibleValue(component.id, component.visible, $event.target.checked)"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My IDE raises a typing error with this syntax.

image

@@ -44,6 +44,7 @@ class Component(BaseModel):
parentId: Optional[str] = None
handlers: Optional[Dict[str, str]] = None
visible: Optional[Union[bool, str]] = None
visibleReversed: Optional[Union[bool]] = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's just a bool, no ?

Suggested change
visibleReversed: Optional[Union[bool]] = None
visibleReversed: Optional[bool] = None

Copy link
Collaborator

@ramedina86 ramedina86 left a comment

Choose a reason for hiding this comment

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

Let's do this without modifying the root of the component.

Avoid having visible and visibleReversed at top level. They're closely related and should form one entity. Reversed it's also not relevant for undefined, true or false.

We can do

visible: {
  "reversed": true,
  "expr": "active"
}

So the TS type would be something like...

boolean | { "reversed": boolean, "expr": string }

@FabienArcellier
Copy link
Collaborator Author

visible: {
  "reversed": true,
  "expr": "active"
}

I was hesitant to make it a single entity. I gave it up. Certainly the 2 attributes live together but they do not have any particular management rule like content could have. These are component parameters.

I would therefore prefer to keep them at the same level as the other parameters of a component because they have the same level of importance. The interest in nested unnecessarily complicates the data structure.

I would rather rename them:

{
	"visibleExpr": ""
	"visibleReverse"
}

Would that be enough?

@ramedina86
Copy link
Collaborator

I'd prefer not, since visibleReversed is only relevant when there's an expression, not when there's undefined, true or false.

Another option can be to include support for ! in the expression evaluator (or hack it and store it inside the expression at the start). I believe the state elements are \w regex so you could easily do that.

@FabienArcellier FabienArcellier force-pushed the 354-checkbox-for-inversion-of-visibility-value branch from 9a5a2d7 to 72ab762 Compare August 12, 2024 13:09
@FabienArcellier
Copy link
Collaborator Author

FabienArcellier commented Aug 12, 2024

I'd prefer not, since visibleReversed is only relevant when there's an expression, not when there's undefined, true or false.

Another option can be to include support for ! in the expression evaluator (or hack it and store it inside the expression at the start). I believe the state elements are \w regex so you could easily do that.

keep the custom value when developer switch visibility. In the visibility part, I had a behavior that frustrated me a lot. When I switch the visibility to Yes, I lose the content that is saved in the custom part. Sometime I just switch to make a check in the IDE.

Would it be possible to treat it this way?

{
	visible: {
		expression: True | False | "custom"
		binding: str
		reversed: bool
	}
}

Peek 2024-08-12 15-23

usage of expression evaluation only if we implement dedicated builder block everywhere. I would advocate to use the expression evaluation only if we create a dedicated builder block we would apply everywhere we manage condition as for the field Show All Option in Pagination for example, .... Otherwise, I would advocate to implement the checkbox.

@ramedina86
Copy link
Collaborator

Makes sense and this looks good, happy to move forward with it.

Let's make sure to make it backwards compatible (I'm guessing should be an easy fix via auditAndFix)

{
	visible: {
		expression: True | False | "custom"
		binding: str
		reversed: bool
	}
}

"""


def fix_components(components: dict) -> dict:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ramedina86 Since the implementation of pydantic, the structure of the ui.json file has been evaluated on the backend side. I had to implement the fix processing when loading the file before it was integrated into pydantic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a good point...

@FabienArcellier FabienArcellier force-pushed the 354-checkbox-for-inversion-of-visibility-value branch 6 times, most recently from f8363dc to dea3c63 Compare August 17, 2024 05:47
@ramedina86
Copy link
Collaborator

Hey, what's the status of this?

@FabienArcellier FabienArcellier force-pushed the 354-checkbox-for-inversion-of-visibility-value branch from dea3c63 to e9f0ecc Compare August 19, 2024 16:07
@FabienArcellier
Copy link
Collaborator Author

Hey, what's the status of this?

I think it's ready to merge

@ramedina86 ramedina86 merged commit 9dc273b into writer:dev Aug 20, 2024
16 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants