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

Draft: Add style property to oh-card #1801

Closed
wants to merge 1 commit into from

Conversation

stefan-hoehn
Copy link
Contributor

@stefan-hoehn stefan-hoehn commented Mar 18, 2023

This adds the style property to an oh-card widget.

The PR is not yet done and is meant to be as proposal to get feedback if this is actually wanted and done the right way.

It adds the style attribute to a card which is currently only also added to the rollershutter card as an example (all other cards would not to be adapted as well). This would allow for example to add a background image to any card which was my original intention. I could have added a background-image property like it is available on the oh-label-card but then I thought I would rather add a general style property which would be more versatile.

I also added a button-style to the rollershutter card itself that would allow additional styling to the buttons (also up to discussion).

in case the property "style" would "collide" with other components "style"-property, we could call this cardStyle instead or something else.

I have seen that people have asked for this now and then but maybe there was a reason not to add it intentionally from the start.

So, let me know what you think

Signed-off-by: Stefan Höhn <mail@stefanhoehn.com>
@stefan-hoehn stefan-hoehn requested a review from a team as a code owner March 18, 2023 15:34
@relativeci
Copy link

relativeci bot commented Mar 18, 2023

Job #869: Bundle Size — 15.66MiB (-3.49%).

67ba1fc(current) vs 2f475cc main#868(baseline)

⚠️ Bundle contains 16 duplicate packages

Metrics (4 changes)
                 Current
Job #869
     Baseline
Job #868
Initial JS 1.72MiB(~+0.01%) 1.72MiB
Initial CSS 608.54KiB(-0.01%) 608.62KiB
Cache Invalidation 95.65% 90.63%
Chunks 219 219
Assets 689(-0.29%) 691
Modules 1705 1705
Duplicate Modules 82 82
Duplicate Code 1.72% 1.72%
Packages 137 137
Duplicate Packages 15 15
Total size by type (4 changes)
                 Current
Job #869
     Baseline
Job #868
CSS 857.81KiB (~-0.01%) 857.89KiB
Fonts 526.1KiB (-52.43%) 1.08MiB
HTML 1.23KiB 1.23KiB
IMG 140.74KiB 140.74KiB
JS 9.18MiB (~-0.01%) 9.18MiB
Media 295.6KiB 295.6KiB
Other 4.7MiB (~-0.01%) 4.7MiB

View job #869 reportView main branch activity

@ghys
Copy link
Member

ghys commented Mar 19, 2023

That would be a cool addition in general.

Instead of adding the property to all card widgets, maybe it's time to do a refactoring I wanted to do for a while:

There is currently no generic oh-card, the oh-*-card widget simply use f7-card in their implementation.
A oh-card that would serve as a base for other card widgets would be nice as it could define a common set of properties that the card itself (as opposed to the widget inside its content) supports - and style and class would be one of them, other renamed style/class properties would be defined for the control inside the card.
This oh-card could also define slots (which the f7-card doesn't do) - header, content and footer - and for instance the oh-rollershutter-card would simply have to define the content slot.
The user would then be able to define the other slots (header and footer) to customize them beyond a simple text.

Lastly a oh-card could be useful even when designing a custom card as it would simplify the syntax.
For example:

component: f7-card
config:
  style: {} # Card style
  class: [] # Card classes
slots:
  default:
    - component: f7-card-header
      config:
        style: {} # Header style
        class: [] # Header classes
      slots:
        default:
          - component: oh-button
            config:
              text: Header
    - component: f7-card-content
      config:
        style: {} # Content style
        class: [] # Content classes
      slots:
        default:
          - component: oh-button
            config:
              text: Content
    - component: f7-card-footer
      config:
        style: {} # Footer style
        class: [] # Footer classes
      slots:
        default:
          - component: oh-button
            config:
              text: Footer

would become:

component: oh-card
style:
  style: {} # Card style
  class: [] # Card classes
  headerStyle: {} # Header style
  headerClass: [] # Header classes
  contentStyle: {} # Content style
  contentClass: [] # Content classes
  footerStyle: {} # Footer style
  footerClass: [] # Footer classes
slots:
  header:
    - component: oh-button
       config:
         text: Header
  content:
    - component: oh-button
       config:
         text: Content
  footer:
    - component: oh-button
       config:
         text: Footer

If the header, content, footer slot contains resp. f7-card-header, f7-card-content or f7-card-footer as the only component they would be inserted directly as defined, otherwise those would be created automatically.
oh-card-footer could be made obsolete as its functionality could be integrated into oh-card.

@stefan-hoehn
Copy link
Contributor Author

Thanks for the feedback, Yannick. Just from your experience: do you think as a first guess that it would be possible without breaking the current widgets that are used by openHAB users?

@florian-h05
Copy link
Contributor

With the experience from my oh-knob refactoring (#1718) I would think that this refactoring of oh-card widgets from f7-card to an oh-card should be possible without introducing breaking changes.

@ghys
Copy link
Member

ghys commented Mar 29, 2023

Yes, I agree.
This could be a nice addition and wouldn't break anything if we're careful, and open new possibilities.
Custom "card" widgets wouldn't be affected because they probably use the current f7-card which won't change; migrating to a oh-card would be at their own discretion.
I might give it a try.

@stefan-hoehn
Copy link
Contributor Author

picked up and superseded by #2781 provided by @florian-h05 🎉

@stefan-hoehn stefan-hoehn deleted the oh_card_style branch October 1, 2024 11:17
florian-h05 added a commit that referenced this pull request Oct 1, 2024
Supersedes #1801.

Implements the proposal from
#1801 (comment).

-----

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
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.

4 participants