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

Allow one level of nested frames in sitemaps #3785

Merged
merged 6 commits into from
Aug 7, 2024

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Jul 28, 2024

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
Copy link
Contributor

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

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

@jimtng
Copy link
Contributor Author

jimtng commented Jul 28, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

jimtng commented Jul 28, 2024

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
Copy link
Contributor

Different colors maybe? colorPrimaryContainer vs. colorSecondaryContainer or colorTertiaryContainer?

@jimtng
Copy link
Contributor Author

jimtng commented Jul 28, 2024

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
Copy link
Member

How does it look with dynamic colors?

@maniac103
Copy link
Contributor

and afaik colorTertiaryContainer doesn't exist.

It does, see here

@jimtng
Copy link
Contributor Author

jimtng commented Jul 28, 2024

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 jimtng marked this pull request as ready for review July 29, 2024 07:12
@jimtng jimtng changed the title Allow nested frames in sitemaps Allow one level of nested frames in sitemaps Jul 29, 2024
Copy link
Member

@mueller-ma mueller-ma left a comment

Choose a reason for hiding this comment

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

I can merge when you resolve the conflict caused by merging the other PR.

@jimtng
Copy link
Contributor Author

jimtng commented Jul 30, 2024

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

@mueller-ma mueller-ma self-requested a review August 1, 2024 15:27
Signed-off-by: Danny Baumann <[email protected]>
(cherry picked from commit ef82e37)
@mueller-ma mueller-ma merged commit 2c7e683 into openhab:main Aug 7, 2024
8 checks passed
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.

Nested frames do not work
3 participants