-
Notifications
You must be signed in to change notification settings - Fork 3
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
Highlight selected page #91
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #91 +/- ##
==========================================
+ Coverage 82.91% 83.30% +0.38%
==========================================
Files 42 42
Lines 5614 5636 +22
Branches 609 768 +159
==========================================
+ Hits 4655 4695 +40
+ Misses 756 740 -16
+ Partials 203 201 -2 ☔ View full report in Codecov by Sentry. |
I really like this feature! I think, I always forgot to implement it, although i consider it necessary for any useful navigation menu. However, in my opinion:
|
I totally agree and this was actually my 1st approach. Here is why it does not work. The problem is the dropdown menu. Semantic uses the We could workaround this with some javascript enhancements, which you actually want to avoid. But still having this in place would not help. Because even if you add/remove the active class accordingly the activated submenu will no longer open since Semantic thinks it is already open because it is active: In my eyes this is a (design) bug in Semantic. I even thought about opening an issue there but the last commit in the Semantic repo was in May and there are 100+ open issues. Looks like the project is no longer actively maintained which could be another problem for us in the long run (also see github/semantic#711). So in the sake of an effective cost/benefit balance I suggest we stick to my proposal w/ an own active class (e.g.
I'm totally fine w/ incorporating this feature into the core class. I will check in another proposal as base for discussion soon. |
test/test_web.py
Outdated
actions = ActionChains(self.driver) | ||
actions.move_to_element(submenu).perform() # do a hoover | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange, with the open on hover in main.js
we need a hover here as well that the tests pass. But seems ok to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like simple click events are no longer sufficient to toggle the dropdown menu. This might have negative effects on the usability, especially when using other input devices (like keyboard, touch screen, digitizer pen). As I already commented, we should make sure that these methods of opening the submenu still work without usability limitations. Then, I'm fine with opening the submenus on hover and adapting this test, if still required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops you are right, forgot about the other devices that you mention. I'll remove this feature as it is not working out of the box for all devices. I thought this may be low hanging fruits but it is not. Thus will be removed.
Ready for review again 😎 Maybe you can give me the rights to assign a reviewer to PRs. So we can use the standard github notification to tell you once I'm done. |
shc/web/templates/base.htm
Outdated
{% set ns = namespace(isSelected=False) %} | ||
{% for sub_label, sub_icon, sub_link in link %} | ||
{% if not ns.isSelected %} | ||
{% set ns.isSelected = (current_url == '/page/' ~ sub_link ~ '/') %} | ||
{% endif %} | ||
{% endfor %} | ||
<div class="mobile hidden ui dropdown item {% if ns.isSelected %}selected{% endif %}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I would like to move all the logic from the jinja template to interface.py
to make it easier to maintain and debug.
In my eyes the best place to temporarily store the isSelected
information would be in the self.ui_menu_entries
. This could look like this:
[('Label', 'icon', 'page_name','is_selected'),
('Submenu label', None, [
('Label 2', 'icon', 'page_name2','is_selected'), ...
]),
...]
What do you think? Any other proposal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'd also prefer to evaluate the active state of the submenu entry in the Python code (this kind of evaluation is one of the annoying things that are often required in Jinja templates but not simple to write due to the missing list comprehension / generator expressions in Jinja).
However, I would probably refactor the code a bit to use a NamedTuple or a Dataclass instead of simple tuples in the list for better readability of both, the Python code and the template code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
web_ui_src/main.js
Outdated
@@ -30,6 +30,6 @@ window.SHC_WIDGET_TYPES = WIDGET_TYPES; | |||
|
|||
// Set up UI with Semantic UI components | |||
$(function() { | |||
$('.main-menu .ui.dropdown').dropdown(); | |||
$('.main-menu .ui.dropdown').dropdown({on: 'hover'}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal preference, quicker for navigation. Let me know if you rather want to have it removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it work reasonably well with touch (or hybrid) devices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature removed, see above.
example/ui_showcase.py
Outdated
menu_sub_label="Empty Submenu") | ||
|
||
submenu_page3 = web_server.page('submenu3', "Nothing here either", menu_entry='Some Submenu', | ||
menu_sub_icon='motorcycle', menu_sub_label="Empty Submenu 2") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also create menu entries without creating actual pages, using add_menu_entry()
.
More specifically, I would suggest to add two links to the same page here for checking the correct behaviour in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Actually the WebServer API page(...)
and add_menu_entry(...)
is kind of confusing and not really obvious, e.g. that in add_menu_entry()
the existing menu_entry is found by the label. Maybe we should align the attribute names in both methods.
page() | add_menu_entry() |
---|---|
menu_entry | label |
menu_icon | icon |
menu_sub_label | sub_label |
menu_sub_icon | sub_icon |
Not sure whether this would have avoided some of my confusion but we should fix it anyhow. Maybe not now since this is an API change or what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm actually quite happy with the current naming of these attributes: The page()
method also takes menu_entry=True
to simply create a menu entry with the page's title as label. This is why the parameter is called this way.
Basically, the page()
method only creates a WebPage and the add_menu_entry()
only adds a menu entry. Both are completely independent from each other. As a shortcut, the page()
method allows to create a matching menu entry for the new WebPage in addition to the WebPage itself, by using the menu_entry
parameter.
Maybe, we should point out this relationship in the docstrings of both methods, with a reference to the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More specifically, I would suggest to add two links to the same page here for checking the correct behaviour in this case.
Well this brings up another issue that we should clarify. Since I identify the current page by the path in that case both menu entries are marked:
This could be a feature or a bug depending on the point of view. From my perspective I would say that is something that could be benefical and that I could live with. Especially if the same page is referenced in different submenus this might be an interesting information being visualized like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm actually quite happy with the current naming of these attributes: The
page()
method also takesmenu_entry=True
to simply create a menu entry with the page's title as label. This is why the parameter is called this way.Basically, the
page()
method only creates a WebPage and theadd_menu_entry()
only adds a menu entry. Both are completely independent from each other. As a shortcut, thepage()
method allows to create a matching menu entry for the new WebPage in addition to the WebPage itself, by using themenu_entry
parameter.Maybe, we should point out this relationship in the docstrings of both methods, with a reference to the other.
Done.
shc/web/templates/base.htm
Outdated
@@ -61,18 +61,26 @@ | |||
<div class="ui container"> | |||
{% for label, icon, link in menu %} | |||
{% if link is string %} | |||
<a class="mobile hidden item" href="{{ root_url }}/page/{{ link }}/"> | |||
<a class="mobile hidden item {% if '/page/' ~ link ~ '/' == current_url %}selected{% endif %}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, the page id should be used instead of the current url. As far as I see, it is currently not available to the template, but it can easily be added to the template data in shc.web.interface.WebServer._page_handler()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed moved the logic to python. Still need to use the url path to identify the current menu. I get this from the request.path
thus I do not know the page id but only the path. Or do I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can simply access the name
attribute of the WebPage
object in the _page_handler()
method. See my comment in shc/web/interface.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep this is perfect. But you mentioned the page id
and I do not have this in my menu structure. But comparing the name is unambiguous so we should be fine.
test/test_web.py
Outdated
actions = ActionChains(self.driver) | ||
actions.move_to_element(submenu).perform() # do a hoover | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like simple click events are no longer sufficient to toggle the dropdown menu. This might have negative effects on the usability, especially when using other input devices (like keyboard, touch screen, digitizer pen). As I already commented, we should make sure that these methods of opening the submenu still work without usability limitations. Then, I'm fine with opening the submenus on hover and adapting this test, if still required.
test/test_web.py
Outdated
self.server.page('index', menu_entry="Home", menu_icon='home') | ||
self.server.page('overview', menu_entry="Foo", menu_icon='info') | ||
|
||
# add submenu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is slightly inaccurate, as these method calls do not only add a submenu, but also new web pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Yes, I see and I agree.
That is why we use the fork Fomantic UI (Web, GitHub) instead of the original Semantic UI. It is not really actively developed, but at least there is some maintenance going on. And I actually contributed two PRs to it to fix some behaviour relevant for SHC, that got reviewed and merged some time ago.
That sounds reasonable. Nevertheless, I would like to make it look like the original |
I don't think this is possible without making you a full collaborator in this repository. See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/requesting-a-pull-request-review:
|
shc/web/interface.py
Outdated
body = await template.render_async(title=page.title, segments=page.segments, menu=self.ui_menu_entries, | ||
root_url=self.root_url, js_files=self._js_files, css_files=self._css_files, | ||
server_token=id(self), html_title=html_title) | ||
self._mark_active_menu_items(request.path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should pass the page name (page.name
) here instead of the full path from the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are so right. Thanks 👍
ok, thanks for pointing it out.
Now using /* **************************
* Menu things
*
* Fomantic-UI active class for menus conflicts w/ the submenu active class usage.
* Thus we define our own `activated` class. Below is a adjusted copy from the Fomantic-UI project.
* See https://github.com/mhthies/smarthomeconnect/pull/91 for details
* ***************************/
/* Menu Items */
.ui.menu .ui.dropdown .menu > .activated.item {
background: rgba(0, 0, 0, 0.03) !important;
font-weight: bold !important;
color: rgba(0, 0, 0, 0.95) !important;
}
/* Vertical */
.ui.vertical.menu .activated.dropdown.item {
border-top-right-radius: 0;
border-bottom-right-radius: 0;
}
.ui.vertical.menu .dropdown.activated.item {
box-shadow: none;
}
/* --------------
Active
--------------- */
.ui.menu .activated.item {
background: rgba(0, 0, 0, 0.05);
color: rgba(0, 0, 0, 0.95);
font-weight: normal;
box-shadow: none;
}
.ui.menu .activated.item > i.icon {
opacity: 1;
}
/* --------------
Active Hover
--------------- */
.ui.menu .activated.item:hover,
.ui.vertical.menu .activated.item:hover {
background-color: rgba(0, 0, 0, 0.05);
color: rgba(0, 0, 0, 0.95);
}
/* Vertical Active */
.ui.vertical.menu .activated.item {
background: rgba(0, 0, 0, 0.05);
border-radius: 0;
box-shadow: none;
}
.ui.vertical.menu > .activated.item:first-child {
border-radius: 0.28571429rem 0.28571429rem 0 0;
}
.ui.vertical.menu > .activated.item:last-child {
border-radius: 0 0 0.28571429rem 0.28571429rem;
}
.ui.vertical.menu > .activated.item:only-child {
border-radius: 0.28571429rem;
}
.ui.vertical.menu .activated.item .menu .activated.item {
border-left: none;
}
.ui.vertical.menu .item .menu .activated.item {
background-color: transparent;
font-weight: bold;
color: rgba(0, 0, 0, 0.95);
} This is quite a lot of stuff and a lot of functionality which we don't need, e.g. changing font and background on hover etc. Also I want to use the same background as our body and not just a grey stuff. So using just my little css code snippet does everything that we need (change the background nothing else). Thus reducing complexity and easier to maintain: .item.activated {
background: url("prism.png") !important;
} Can you live with it? |
All done, waiting for your feedback/approval 😉 |
fix: doc
shc/web/interface.py
Outdated
# ('Label 2', 'icon', 'page_name2'), ... | ||
# [('Label', 'icon', 'page_name', 'is_active'), | ||
# ('Submenu label', 'icon', None, 'is_active' [ | ||
# ('Label 2', 'icon', 'page_name2', 'is_active'), ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should be adjusted for the new classes, which replace the old tuple-based structure. I.f.
# The structure looks as follows:
# [MenuEntrySpec('Label', 'icon', 'page_name', false),
# SubMenuEntrySpec('Submenu label', 'icon', None, true, [
# MenuEntrySpec('Label 2', 'icon', 'page_name2', true), ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're so right. Fixed.
shc/web/interface.py
Outdated
else: | ||
sub_item.is_active = False | ||
else: | ||
item.is_active = page_name == item.page_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't like that this method changes the is_active
attribute of the MenuEntrySpecs in the (server-global) self.ui_menu_entries
attribute, such that this is now a side-effect of the _page_handler()
method.
This may even result in concurrency issues, considering, that template.render_async()
may be executed concurrently for multiple concurrent requests. So, the is_active
value should definitely only be changed in a local data structure within the _page_handler()
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, understood & fixed.
Hm, actually not. I consider UX consistency quite important and want to stay as close as possible to the overall design, as thought out by the Semantic UI designers. So, when styling is already included, for the exact semantics that we want to express, we should use it – or mimic it, as close as possible. In addition, I personally consider reusing the background image as quite unaesthetic, as it looks to me like a gap in the menu bar. As far as I can see, the |
Can you do me a favor and test this locally on your site. Does it work or do you get the same errors as I do? |
Active | ||
--------------- */ | ||
.ui.menu .activated.item { | ||
background: rgba(50, 50, 50, 0.8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this is a little bit lighter as w/ the values from fomatic it was really hard to see the difference.
} | ||
|
||
.ui.menu .ui.dropdown .menu > .selected.item { | ||
background: rgba(0, 0, 0, 0.15) !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed transparency as w/ the values from fomantic it was really hard to see the difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, now! :)
My sincere thanks for addressing my overly concrete expectations!
I'll merge this PR without squashing, to make the different steps visible in the Git history. If you like, you can interactively rebase it before, to squash all the fixup commits together (or into the actual changes). If not, I'm also fine with merging it like this. :)
😄 Fine for me, whatever you prefer. Feel free to merge as it is. |
Any feedback on this issue for me? If it runs on your local system I need to investigate further here on my side. But not really sure. Thanks. |
Ah, sorry, I forgot about that. On my computer, the coverage/unittest command runs successfully. Could it be that you are using a virtualenv for the SHC dependencies but invoking the |
Uh, I wouldn't have thought that the selenium-webdriver behaves differently in WSL. I think, we can add a workaround to optionally reinitialize the driver for every test, using an environment variable to switch the behaviour. But you are right, we probably should discuss this in a new issue. |
Even better, here is the PR: #99 Let's discuss it there 😃 |
Here is a minor improvement from which others may also benefit. Once you select a menu item in the web it stays highlighted.
Also the submenu item is highlighted:
Since it was not obvious to me how to add multiple entries to a submenu and since I needed those for testing I added two additional entries in the
ui_showcase.py
. Furthermore wrote a test to check all the voodoo.If you like this change just let me know whether it is implemented in the correct location (not sure about it) otherwise feel free to reject this PR.