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

Multiselect-dropdown-foreground-background #590

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Duncanr-glitch
Copy link

Implements Feature #380 for background and foreground of the multiselect dropdown.

The same foreground and background are applied to the exterior and interior of the dropdown, as well as the checked icon.

The popover can also now have a background since this is how the multiselectdropdown is implemented

Copy link
Collaborator

@s-cork s-cork left a comment

Choose a reason for hiding this comment

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

Just some initial thoughts from looking at the code. Won't be able to properly try out these PRs till the New Year

Comment on lines 86 to 94
@property
def foreground(self):
return self._foreground

@foreground.setter
def foreground(self, value):
self._foreground = value
self.label.foreground = self.icon_checked.foreground = value

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we follow the code style of the component and keep props in the _props dict.

Same for other components in this PR.

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -330,6 +334,9 @@ def init_popover(self, element):
element.setAttribute("ae-popover", "")
element.setAttribute("ae-popover-id", self.id)

if self.background:
_S(element).css("background-color", self.background)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not use jquery when vanilla JavaScript will do just fine. Likewise for other uses of jquery

Copy link
Author

Choose a reason for hiding this comment

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

done

@foreground.setter
def foreground(self, value):
self._foreground = value
if value and self._props_initialized:
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't seem like it's possible to remove background/foreground

Copy link
Author

Choose a reason for hiding this comment

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

did that by just removing the conditional checks (for the most part)

Comment on lines 239 to 250
pop(self._select_btn, "destroy")
popover(
self._select_btn,
self._dd,
placement="bottom-start",
arrow=False,
delay=0,
animation=False,
trigger="manual",
max_width="fit-content",
background=value,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels a bit of a strange place to do popover creation.

Copy link
Author

Choose a reason for hiding this comment

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

I fixed this by changing the popover so that the popover function returns the Popover object and adding background changing to the fui module's auto_update function

Duncanr-glitch and others added 4 commits December 25, 2024 08:51
2. used the _props dict where relevant
3. Modified popover and fui so I could change the background color without recreating
4. background/foreground can now be removed
5. items can be added with the write foreground color
Comment on lines 302 to 309
fui.auto_update(
_get_popper_element(self.popper),
self.dom_popover,
placement=self.placement,
arrow=self.dom_arrow if self.arrow else None,
foreground=self.foreground,
background=value,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think floating ui needs to handle background or foreground. We give fui the floating element so the Popover class can handle background and foreground and fui doesn't need to get involved.

Copy link
Author

Choose a reason for hiding this comment

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

That's true, I hadn't considered that, but just simplified it with that :)

…o the popover file;

removed unrelated changes to progress bars from other pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants