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

Add slot for slides to oh-swiper, fixes #840 #1002

Merged
merged 1 commit into from Apr 18, 2021
Merged

Conversation

hubsif
Copy link
Contributor

@hubsif hubsif commented Apr 16, 2021

Looking into issue #840, the anlaysis given (init of swiper before population) seemed plausible and similar similar to #967.

Since the app cannot have any influence in widgets created with f7 components only, I checked why it wasn't possible use oh-swiper.
I found that oh-swiper does not accept slides as child components but wraps children in slides. That way it's not possible I think to use oh-repeater.
So, to make that possible and to possibly run a swiper.update after mount (similar to #967) I adjusted oh-swiper and added a new slot slides, which expects child components of type oh-swiper-slide or f7-swiper-slide.

Luckily, with this no swiper.update is even required - it simply works. The sample given in #840 now works without any resize required (e.g. "1|2|3" as "reparray"):

uid: repeater_bug
tags: []
props:
  parameters:
    - description: Repeater Array
      label: Repeater Array
      name: reparray
      required: true
      type: TEXT
  parameterGroups: []
timestamp: Apr 16, 2021, 1:31:34 PM
component: f7-card
slots:
  content:
    - component: oh-swiper
      config:
        scrollbar: true
        params:
          slidesPerView: 1
          direction: vertical
        style:
          width: 200px
          height: 200px
      slots:
        slides:
          - component: oh-repeater
            config:
              for: repeat
              in: =props.reparray.split("|")
              fragment: true
            slots:
              default:
                - component: f7-swiper-slide
                  slots:
                    default:
                      - component: f7-col
                        config:
                          class:
                            - display-flex
                            - flex-direction-column
                            - align-items-center
                        slots:
                          default:
                            - component: f7-block-title
                              slots:
                                default:
                                  - component: Label
                                    config:
                                      text: =loop.repeat

So, in the end this fixes the display issue and allows for oh-repeater to be used in oh-swiper.

@hubsif hubsif requested a review from a team as a code owner April 16, 2021 17:09
@relativeci
Copy link

relativeci bot commented Apr 16, 2021

Job #79: Bundle Size — 10.45MB (~-0.01%).

30a61aa vs 42e8af8

Changed metrics (2/8)
Metric Current Baseline
Initial JS 1.61MB(~-0.01%) 1.61MB
Modules 1442(-0.07%) 1443
Changed assets by type (1/7)
            Current     Baseline
JS 8.12MB (~-0.01%) 8.12MB

View Job #79 report on app.relative-ci.com

Signed-off-by: Hubert Nusser <hubsif@gmx.de>
Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

Note that observer: true in the config seems to allow a oh-repeater with fragment: true in the default slot of a regular f7-swiper, ie. this apparently works:

uid: repeater_bug
tags: []
props:
  parameters:
    - description: Repeater Array
      label: Repeater Array
      name: reparray
      required: true
      type: TEXT
  parameterGroups: []
timestamp: Apr 16, 2021, 1:31:34 PM
component: f7-card
slots:
  content:
    - component: f7-swiper
      config:
        scrollbar: true
        pagination: true
        params:
          slidesPerView: 1
          observer: true
        style:
          width: 200px
          height: 200px
      slots:
        default:
          - component: oh-repeater
            config:
              for: repeat
              in: =props.reparray.split("|")
              fragment: true
            slots:
              default:
                - component: f7-swiper-slide
                  slots:
                    default:
                      - component: f7-col
                        config:
                          class:
                            - display-flex
                            - flex-direction-column
                            - align-items-center
                        slots:
                          default:
                            - component: f7-block-title
                              slots:
                                default:
                                  - component: Label
                                    config:
                                      text: =loop.repeat

The oh-swiper and oh-swiper-slide components' original purpose was just to add design-mode controls to their f7 equivalents ("+" placeholder, context menus).
If you're not making use of these features (that is, you're developing a widget in YAML) then unless I'm mistaken there little benefit to use these components over the f7 ones - just remember to add observer: true when the slides are computed dynamically.

Still this PR doesn't hurt so LGTM.

@ghys ghys merged commit b268450 into openhab:main Apr 18, 2021
@hubsif
Copy link
Contributor Author

hubsif commented Apr 18, 2021

Note that observer: true in the config seems to allow a oh-repeater with fragment: true in the default slot of a regular f7-swiper

Oh, actually didn't think of fragment: true and didn't really know about observer. And yes, that seems to work just fine!

The oh-swiper and oh-swiper-slide components' original purpose was just to add design-mode controls to their f7 equivalents ("+" placeholder, context menus).

Right, so there's actually no point in adding this PR. On the contrary, it might encourage widget designers to use oh-swiper when they shouldn't (and esp. oh-swiper I think should not be used, as like you mentioned it adds that extra placeholder when layouting a page, and being shown for a custom widget can be quite misleading) ...

@hubsif hubsif deleted the fix_840 branch April 20, 2021 19:11
hubsif added a commit to hubsif/openhab-webui that referenced this pull request May 6, 2021
Signed-off-by: Hubert Nusser <hubsif@gmx.de>
@ghys ghys added this to the 3.1 milestone May 30, 2021
@ghys ghys added main ui Main UI enhancement New feature or request labels May 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants