openhab / openhab-android

openHAB client for Android
https://play.google.com/store/apps/details?id=org.openhab.habdroid
Eclipse Public License 2.0
606 stars 313 forks source link

Allow one level of nested frames in sitemaps #3785

Closed jimtng closed 3 months ago

jimtng commented 4 months ago
image
sitemap nestedframe {
  Frame label="outer" {
    Frame label="inner1" {
      Text label="Text inside inner1"
    }
    Frame label="inner2" {
      Text label="Text inside inner2"
    }
  }
  Frame label="outer too" {
    Text label="Inside outer too"
  }
}

I learned that nested frames aren't "supposed" to be supported, but BasicUI and iOS renders them, whilst Android doesn't.

Here, I propose to render nested frames, and also provide a visual clue for the nesting structure by using colorTertiaryContainer, indenting the header text and reduce the header bg's height/padding, as suggested by @maniac103

Resolve #324, #1639, #2804 Also #420

maniac103 commented 4 months ago

Nice to see this implemented (even though nested frames still aren't fully supported, but only one level), but I think the visual representation can use some improvement. My suggestion would be using a separate view (and thus view holder) where

jimtng commented 4 months ago

even though nested frames still aren't fully supported, but only one level

It's a tricky one. If we support unlimited nesting levels, we'd need to have a way to provide visual clues about the nesting levels. Simply indenting the "header" is going to look weird when the "content" of the frames will remain at the same left indentation. So to do that, we'd need to indent all the children too, which requires a bit of a restructure to the way Frames are currently done.

maniac103 commented 4 months ago

Just to be clear: I don't want to support unlimited nesting, precisely for the reasons you outlined: we'll run out of screen real estate pretty fast. Personally I'm not even convinced that two level nesting is a good idea, but I know there's quite a few people thinking otherwise.

jimtng commented 4 months ago

I think the visual representation can use some improvement. My suggestion would be using a separate view (and thus view holder) where

  • the background still starts at the left edge
  • the text is indented and
  • the background is a bit more narrow (less padding)

I tried this

image

However, I think the difference is a bit too subtle. How can we improve it?

maniac103 commented 4 months ago

Different colors maybe? colorPrimaryContainer vs. colorSecondaryContainer or colorTertiaryContainer?

jimtng commented 4 months ago

Different colors maybe? colorPrimaryContainer vs. colorSecondaryContainer or colorTertiaryContainer?

Tried this. Only BasicUI Color Scheme has those two different. Other color schemes use the same color for them, and afaik colorTertiaryContainer doesn't exist.

image
mueller-ma commented 4 months ago

How does it look with dynamic colors?

maniac103 commented 4 months ago

and afaik colorTertiaryContainer doesn't exist.

It does, see here

jimtng commented 4 months ago

Ahh, thanks! tertiary looks better, although we might need to tweak it? I totally suck at choosing colours though.

image image

Basic UI

image

Dynamic

image
jimtng commented 4 months ago

Rebased to main. Note the change from secondary to tertiary container color was also added.