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

Introduce QSlideItem element #4358

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

Conversation

thetableman
Copy link
Contributor

@thetableman thetableman commented Feb 16, 2025

This PR introduces a Slide Item element based on Quasar's QSlideItem https://quasar.dev/vue-components/slide-item

@falkoschindler and @rodja I would be keen to hear your feedback on how I handled the LeftSlide, RightSlide, etc relationship with SlideItem, I feel there may be a better way to add the association of their slot to the parent element.

I'm also interested to hear your feedback on whether an on_change callback is really needed if each slot already has an on_slide callback.

I was undecided whether creating LeftSlide, RightSlide, etc classes were truly necessary, a user could just use SlideSide with the side parameter, but noted the style of Drawer, LeftDrawer, RightDrawer so aligned to this.


Feature request: #4282

@falkoschindler falkoschindler self-requested a review February 17, 2025 05:59
@falkoschindler
Copy link
Contributor

Hi @thetableman, thanks for yet another awesome pull request!

Just some quick feedback (haven't looked at all the code yet):

  • Instead of nesting ui.item_section as a direct child of ui.slide_item, I think the usage should be
    with ui.list():
        ui.item('Item A')
        ui.item('Item B')
        with ui.slide_item():
            ui.item('Slide me!')
            with ui.left_slide(color='green'):
                ui.item_label('Left')
            with ui.right_slide(color='red'):
                ui.item_label('Right')
    Using ui.item inside ui.slide_item() yields a consistent layout with regular items.
  • Regarding the API for individual slots: What about creating slots like ui.splitter does with before and after? This way we only need to define a single UI element and the usage is pretty straight forward. Ok, we can't easily define colors for slots. But maybe we should simply add left_color, right_color, ... parameters to ui.slide_item and we're good. This might be more accessible and more aligned with QSlideItem.

@falkoschindler
Copy link
Contributor

Maybe we can make the predefined slots callable to assign color and event handler...

with slide_item.left(color='green', on_slide=lambda: ui.notify('Left')):
    ui.item_label('Left')
with slide_item.right(color='red', on_slide=lambda: ui.notify('Right')):
    ui.item_label('Right')

Or left and right are methods that add these slots when called. So many possibilities... 😄

@thetableman
Copy link
Contributor Author

thetableman commented Feb 18, 2025

Thanks for reviewing @falkoschindler

Instead of nesting ui.item_section as a direct child of ui.slide_item, I think the usage should be

Are you suggesting here that a ui.item_section becomes the default slot when a ui.slide_item is created? If so, this was my original intention however navigating the slot stack to ensure that the nested ui.left_slide was in the right place was messy.

Regarding the API for individual slots: What about creating slots like ui.splitter does with before and after? This way we only need to define a single UI element and the usage is pretty straight forward. Ok, we can't easily define colors for slots. But maybe we should simply add left_color, right_color, ... parameters to ui.slide_item and we're good. This might be more accessible and more aligned with QSlideItem.

I did play with this idea too but ultimately found it restrictive.

  • Slide sides are optional - you should be able to have a 'slide left' action only if desired, adding all slide slots on creation means they exist until you remove.
  • Further to point one, you mention 'left' and 'right' but there are also 'top' and 'bottom'. These are likely to be used less often but exist as options all the same.
  • Further to point two, adding color options to ui.slide_item would then need four color parameters, one for each side. I'm not a fan of parameters being available for potentially redundant slots.
  • To avoid four SlideSide inherited classes then I would opt for just using SlideSide and specifying side.

Maybe we can make the predefined slots callable to assign color and event handler...

I think I like this alternative approach best but wouldn't this example create two ui.slide_item elements, one with a left and one with a right, rather than a single ui.slide_item with both? It would reduce the number of elements, dependent specifically on a single element, appearing in ui, which I support.

Surely usage would need to be as follows... but then this is almost as before:

with ui.slide_item as slide_item:
    ui.item('Slide me')
    with slide_item.left(color='green', on_slide=lambda: ui.notify('Left')):
        ui.item_label('Left')
    with slide_item.right(color='green', on_slide=lambda: ui.notify('Right')):
        ui.item_label('Right')

So many possibilities... 😄

Indeed! I had a lot of fun approaching this one. For such a simple element there are a lot ways to approach.

@thetableman
Copy link
Contributor Author

I've had a play with two different approaches to the ui.slide_item.left suggestion based on the SlideSide classes defined in the PR. I'm not sure which I prefer at this stage.

class SlideItem(DisableableElement):
    def __init__(self,
                 on_change: Optional[Handler[ClickEventArguments]] = None):

        super().__init__(tag='q-slide-item')

        self._active_slides: list[str] = []

        if on_change:
            self.on_change(on_change)

        self._left_slide: LeftSlide = None
        

    class right_slide(SlideSide):
        def __init__(self,
                 on_slide: Optional[Handler[GenericEventArguments]] = None,
                 color: Optional[str] = 'primary',
                 ) -> None:
            super().__init__('right', color=color, on_slide=on_slide)

    @property
    def left_slide(self) -> LeftSlide:

        if self._left_slide:
            return self._left_slide
        
        self._left_slide = LeftSlide
        
        return self._left_slide

    def reset(self) -> None:
        """Reset the Slide Item to initial state"""
        self.run_method('reset')
from nicegui import ui

with ui.list().props('dense separator'):
    with ui.slide_item() as slide_item:
        with ui.item_section().classes('h-20'):
            ui.item_label('Slide me Up or Down')
        with slide_item.left_slide(on_slide=lambda: ui.notify('Left')):
            ui.item_label('Left')
        with slide_item.right_slide(on_slide=lambda: ui.notify('Right')):
            ui.item_label('Right')

ui.button('Reset Slide Item', on_click=lambda: slide_item.reset())

ui.run(reload=False)

@falkoschindler
Copy link
Contributor

Are you suggesting here that a ui.item_section becomes the default slot when a ui.slide_item is created?

No, I was referring to your main demo. If ui.item_section is a direct child of ui.slide_item, the layout looks off. And according to Quasar's documentation, this is not the intended use of QSlideItem. Instead the ui.item_section should be wrapped inside another ui.item. Or we use NiceGUI's text argument for ui.item which does the rest for us:

ui.item('Item A')

Slide sides are optional - you should be able to have a 'slide left' action only if desired, adding all slide slots on creation means they exist until you remove.

Good point! We don't want them to be always created.

Further to point one, you mention 'left' and 'right' but there are also 'top' and 'bottom'. These are likely to be used less often but exist as options all the same.

Yes, ignored "top" and "bottom". But they are a good example for optional slots that definitely need to stay optional.

Further to point two, adding color options to ui.slide_item would then need four color parameters, one for each side. I'm not a fan of parameters being available for potentially redundant slots.

Agreed.

but wouldn't this example create two ui.slide_item elements, one with a left and one with a right, rather than a single ui.slide_item with both?

Ah, I omitted the wrapping ui.slide_item(). Calling slide_item.left() and slide_item.right() would only add slots but no additional slide items.

So I think we can agree on a usage like this:

with ui.slide_item as slide_item:
    ui.item('Slide me')
    with slide_item.left(color='green', on_slide=lambda: ui.notify('Left')):
        ui.item_label('Left')
    with slide_item.right(color='green', on_slide=lambda: ui.notify('Right')):
        ui.item_label('Right')

I've had a play with two different approaches to the ui.slide_item.left suggestion based on the SlideSide classes defined in the PR. I'm not sure which I prefer at this stage.

I think we can simplify it by defining left and right as methods. They can directly refer to self and don't need to find their parent via _get_slide_parent. Actually, I think we can remove the SlideSide class as well as their subclasses LeftSlide, RightSlide etc.

Indeed! I had a lot of fun approaching this one. For such a simple element there are a lot ways to approach.

Oh yes, I love tinkering with such API decisions as well. Often you end up with nice solutions you haven't thought of in the beginning. And ultimately those solutions make a great library. 🙂

@thetableman
Copy link
Contributor Author

No, I was referring to your main demo. If ui.item_section is a direct child of ui.slide_item, the layout looks off. And according to Quasar's documentation, this is not the intended use of QSlideItem. Instead the ui.item_section should be wrapped inside another ui.item

Ah, yes, you're right!

How about this approach with methods then? We can add left(...), right(...), etc methods if needed but as a working concept:

class SlideItem(DisableableElement):
    def __init__(self,
                 on_change: Optional[Handler[ClickEventArguments]] = None):

        super().__init__(tag='q-slide-item')

        self._active_slides: list[str] = []

        if on_change:
            self.on_change(on_change)

    def slide(self,
              side: SlideSides, *,
              on_slide: Optional[Handler[GenericEventArguments]] = None,
              color: Optional[str] = 'primary',
              ) -> Self:
        
        if color:
            self._props[f'{side}-color'] = color

        if on_slide:
            self.on_slide(side, on_slide)

        if side not in self._active_slides:
            self._active_slides.append(side)

        return self.add_slot(side)

        
    def on_change(self, callback: Handler[ClickEventArguments]) -> Self:
        """Add a callback to be invoked when the Slide Item is changed."""
        self.on('action', lambda _: handle_event(callback, ClickEventArguments(sender=self, client=self.client)))
        return self
    
    def on_slide(self, side: SlideSides, callback: Handler[GenericEventArguments]) -> Self:
        """Add a callback to be invoked when a Slide Side is activated."""
        self.on(side, lambda _: handle_event(callback, GenericEventArguments(sender=self, client=self.client, args=side)))
        return self

    def reset(self) -> None:
        """Reset the Slide Item to initial state"""
        self.run_method('reset')

    @property
    def active_slides(self) -> list:
        """Returns a list of active Slide Sides"""
        return self._active_slides
from nicegui import ui

with ui.list().props('dense separator'):
    with ui.slide_item() as slide_item:
        ui.item('Slide Me Left or Right').classes('h-20')

        with slide_item.slide(side='left', color='positive', on_slide=lambda: ui.notify('Left')):
            ui.label('Left')
        
        with slide_item.slide(side='right', color='negative', on_slide=lambda: ui.notify('Right')):
            ui.label('Right')

ui.button('Reset Slide Item', on_click=lambda: slide_item.reset())

ui.run()

@falkoschindler
Copy link
Contributor

@thetableman Looks great! 👍🏻

@falkoschindler falkoschindler added the enhancement New feature or request label Feb 25, 2025
@thetableman
Copy link
Contributor Author

Done!

I'll leave it to you to decide whether creating 'left', 'right', etc methods are necessary.

@falkoschindler
Copy link
Contributor

Nice! Quick question: What is the use case for the active_slides property? If in doubt, I'd like to remove it. If you really need this information in rare cases, there's still the slots dictionary.

@falkoschindler falkoschindler added this to the 2.12 milestone Feb 26, 2025
@thetableman
Copy link
Contributor Author

You can remove active_slides if you don't see value. I just prefer to include additional information where relevant and the ability to check what sides are active in the element easily without navigating the slots dict may be beneficial. It's not a deal-breaker though, it's just an extra tool for user's should they need it.

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.

2 participants