-
-
Notifications
You must be signed in to change notification settings - Fork 243
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
[Basic UI] Improve layout of header and buttons #2065
Comments
No, this is exactly the general principle of any sitemap element. If a label/icon is set on the sitemap element, we use it. If not set and if the sitemap element is linked to an item and if a label/icon is set for this item, we use the item label/icon for the sitemap element. And finally if still no label/icon set on the item, we use the item name as label and the icon associated to the sitemap element as icon (chart.svg in the current case).
I have no idea how to implement that overlay stuff. What I can propose for those of you who do not like this new header line for chart and image elements is to do something similar to what I have already done for the video/webview/mapview elements, that is the header line is hidden in case an empty label is set on the sitemap element. This change is trivial to implement, just few lines of code. Like that, everybody can restore the old behaviour if he prefers to not have the header line for charts. There will be only one exception when an image item is part of a group and you open the group, the default will be to display the header line for the image. As chart is never the default element of an item, you will never have chart elements when opening a group of items. Another option will be to enhance the sitemap syntax for chart and image elements to let the user adding "withHeader" when he wants the header line. In that case, the default will be to show the header line. In that case, this new property should be also handled in Android and iOS app. WDYT ? |
Yes, you're right. But then again, chart, image and webview widgets were never really meant to have a header. What I like is the solution in the Android app: If a user is interested in the details of a specific chart/image, they can click on it, get it full screen and then have the buttons to toggle the legend or to select a different time range. Couldn't this be an option for the Basic UI as well? |
At the origin, there were issues/requests declared by users asking icon and label for any sitemap element including chart. So as usual we have a part of users requesting something and others who don't want it. What is done in the Android app for chart (version in google store) is an option. IMHO having to click on the chart to open a new page to see the icon/label is a little strange and certainly not what was requested. |
The Android app certainly works differently than BasicUI here:
Having mentioned the last point, it rather should read 'should be smaller', because it currently isn't: I think I'd prefer a layout which we previously used for Mapviews: That way it becomes more obvious label and icon belong to the chart/image/... and are not just another widget without state. As for 'some people want labels for their charts and some don't': maybe a compromise could be not auto-populating the label field for those widgets in the server, that is, only fill out the widget label if explicitly listed in the sitemap and do not fall back to the item label? That way, the default view would still look as before, while still providing the option to show label and icon if desired. For the apps, there's no way to differentiate 'user has set a label on the sitemap widget' and 'server filled out label from item label as widget label was not provided for sitemap widget' anway, as all we have to work with is the
TBH, this sounds a bit overengineered to me. So far, the sitemap syntax is mostly free of UI-specific toggles, and I think that's a good thing. The way I could envision this working in BasicUI is
|
In the case of BasicUI, wouldn't a popup/dialog kind of thing not be better than navigating to a new page? |
I think @maniac103's suggestion is the way to go:
This way there's no need for additional configuration and it's backwards compatible to existing Sitemaps. |
We have to be clear for which sitemao elements we introduce this exception.
You mean if icon is not defined on the sitemap element?
Ok To summarize, the idea is that the user will have to set a label to its sitemap element to get the header line. That is a reasonable suggestion I believe. |
Two options:
The former is 100% backwards compatible (since it will hide the header where not present before, and where thus label wasn't used before), while the logic of the latter is more straightforward, but needs client changes (even though it's a simple one in the case of the Android app at least).
Correct. |
I agree the first option will in practice break nothing as the label for these elements were not displayed before. I do not understand your second option. The inheritance of label/icon from item for other elements is very important and we clearly do not want to break that feature. |
Or by "all of them", you mean chart, image, map, video, webview but not all sitemap elements ? |
I meant literally all of them. The documentation doesn't mention whether that replacement is done on server or client side ;-) |
I am not sure to understand what you want to do with your option 2. IMHO, the label should be computed by the core framework for all UIs and provided with the REST API. If each UI starts developing its own logic, that is not consistent for the end user. |
The advantage of option 2 would be all sitemap widgets behaving exactly the same, there would be no special cases (in the server and JSON response). |
I have suddenly another idea which is a kind of compromise with Kai's idea: the header line for image and chart elements would be hidden by default but clicking on the image will alternatively show and hide this header line. So without any click, you will still get the historic rendering without icon/label/buttons but just with one click on the image you will then get the additional line. Without feedback, I will go in that direction. |
The Android app now still shows a header bar (as per my second screenshot above) while it didn't do that before and which BasicUI also wouldn't show by default with that proposal, so if that were the new BasicUI behavior, we'd need to revert the respective app commit. I assume you didn't like the idea given towards the end of #2065 (comment) ? If so, any specific reason why? Personally I still think this + depopulated label if not given in sitemap is the best way to move forward. |
I like it less than my last proposal for 3 reasons:
|
I don't agree with 2, since the presence of the label just controls how exactly the 4 options are displayed, it doesn't control whether they are present or not. Such kind of readjustment for space saving reasons doesn't seem too unusual. For 1, one could also think of a client heuristic like 'if widget and item label match, assume widget label was populated from item label'. Alternatively, there could be an attribute like Item 3 really should not be a valid argument. If the code reaches a complexity level where nobody dares to touch it anymore, it should be refactored immediately ;-) |
Yes, I know, it was more a joke, that is why I ended it with a smiley. But it is also true that the code is complicated on this part. It was changed in OH4 and a regression was introduced (hopefully quickly fixed through one of the hot fix). |
If I understand this correctly, this is very much what I proposed above. When opening a page, it "looks" like before (i.e. backward compatible, without a header line) and by clicking on it, the details and buttons appear. That's what my Android app version (3.7.0) is doing and what I referred to above.
If the Android code has meanwhile changed, maybe it would be indeed a good idea to revert it, so that the behavior is similar. |
I'm not sure a straight revert is the best idea given people actually asked for an option for showing a label for charts:
While I agree we should aim at similar behavior between UIs and at not showing the label by default, I still think one of the proposals in
makes the most sense, as it allows the UIs finer control about when to display the header. Assuming the latter proposal would be implemented, I'd make the Android app show the label if it's populated from the sitemap widget definition and hide it if that's not the case. @kaikreuzer WYDT? |
Personally, like Kai, I prefer largely my last proposal, which is very simple, providing exactly the same rendering as before by default, without changing anything (I mean nothing to change by the user), which does not introduce any special/uncommon handling for the label and which makes the presence of the header line not dependent on the definition of a label. |
... and which ignores the user requests to optionally show the label, because it never shows them. It's not that that proposal only has advantages.
The thing is, we already do have special handling for the label, in that we do not show it for charts, images etc. at all currently. |
Well, if you read the issue closely, he reported a bug that label+icon are not considered, then noticed that the label IS shown when clicking on the chart and hence commented in the issue that only the icon is ignored. This sounds to me as if he is happy with the solution to only show them in the "detail" page after clicking the chart.
Yes, this one would also be my favorite (and very similar to my initial thoughts on top), but @lolodomo mentioned that he wouldn't know how to implement this for Basic UI (and I wouldn't know either...). |
It was my understanding that statement referred to the proposed overlay of the buttons when hovering the chart/image. That's why I suggested the 'settings' icon with menu as a more static alternative. IMO the implementation of the header bar does not necessarily need to be tied to the decision about whether or not to show label and icon if the label is specified in the sitemap. #1367 (comment) shows that you had the same thoughts as now 2 years ago already ;-)
That's correct about the Android report and #1931 (made by the same person), but not about #1367. |
@lolodomo Wdyt? Is such a static overlay possible? I think that would be a good compromise.
Ouch, you're not fair! 😛 |
The label can be populated from a label specified on the respective sitemap widget, or (if no label was specified) from the label of the backing item. Allow clients to differentiate between both cases. Related to openhab/openhab-webui#2065 Signed-off-by: Danny Baumann <dannybaumann@web.de>
I will be honest, I am now totally lost, I do not understand what is the solution you are both converging to ! Kai, your request is to restore the old rendering by default, that is no icon/title and no buttons by default and without the need to update all user sitemaps. Do we at least all agree on that goal ? |
Kai, what is the problem for you exactly? Is it the presence of the header line in general or is it only its current size ? |
I think Kai's main goal is to not show the header bar by default. As the functionality makes sense, though, I think the suggestion in #2065 (comment) would be a good compromise, if implementable. Putting the icon bar to the right of the image still would show it rather prominently, which is why I suggested the small icon + menu thing. |
The label can be populated from a label specified on the respective sitemap widget, or (if no label was specified) from the label of the backing item. Allow clients to differentiate between both cases. Related to openhab/openhab-webui#2065 Signed-off-by: Danny Baumann <dannybaumann@web.de>
@kaikreuzer : are you there? |
Yes, I agree with this. Wrt the buttons, the referred suggestion is also very nice:
This was the part I referred to when asking your @lolodomo whether this small "settings overlay" is feasible to implement or not. |
I have no clear idea how to handle overlay and I will not spend several days to search how to do that, at least not in the coming weeks. As a short and quick solution, I will make the header line hidden if there is no "label" explicitly set on the sitemap element for Image/Chart/mapview/Video/Webview elements. |
Applied to Chart/Image/Viodeo/Mapview/Webview elements. Chart and Image elements are also now clickable. Clicking allows toggling between header row shown and header row hidden. Fix openhab#2065 Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Applied to Chart/Image/Viodeo/Mapview/Webview elements. Chart and Image elements are also now clickable. Clicking allows toggling between header row shown and header row hidden. Fix openhab#2065 Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Applied to Chart/Image/Viodeo/Mapview/Webview elements. Chart and Image elements are also now clickable. Clicking allows toggling between header row shown and header row hidden. Fix #2065 Signed-off-by: Laurent Garnier <lg.hc@free.fr>
The label can be populated from a label specified on the respective sitemap widget, or (if no label was specified) from the label of the backing item. Allow clients to differentiate between both cases. Related to openhab/openhab-webui#2065 Signed-off-by: Danny Baumann <dannybaumann@web.de>
…3804) * [sitemap] Provide information about widget label source to clients The label can be populated from a label specified on the respective sitemap widget, or (if no label was specified) from the label of the backing item. Allow clients to differentiate between both cases. Related to openhab/openhab-webui#2065 Signed-off-by: Danny Baumann <dannybaumann@web.de>
This refers to #2010 and the discussion at the forum:
After seeing the new feature on my production env, I feel that it could possibly be improved. What was a neat, slim list of charts before has now become bloated and imho unpleasantly arranged:
Things that might be improved:
label=""
andicon="none
does not feel right, it is counter-intuitive (we usually define what we want and not what we do not want) and looks rather like a workaround.Looking forward to your thoughts on this, @lolodomo.
The text was updated successfully, but these errors were encountered: