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.

Sign up for GitHub

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

Merged
merged 24 commits into from
Nov 30, 2024
Merged

Highlight selected page #91

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
130250e
add: highlightening selected menu item
jo47011 Oct 31, 2024
8e90389
remove css part, now using semantic defaults
jo47011 Oct 31, 2024
2a309c4
nop: try trigger github actions
jo47011 Oct 31, 2024
67d03df
add: test for menu selection
jo47011 Oct 31, 2024
97ed155
fix: flake and mypy issues
jo47011 Oct 31, 2024
1d1d5c5
fix: flake and mypy issues
jo47011 Nov 3, 2024
85f6b4a
remove: legacy code
jo47011 Nov 3, 2024
5f68432
fix test: add hover action
jo47011 Nov 3, 2024
2125048
fix test: add hover action
jo47011 Nov 3, 2024
6c5d216
move: active logic from jinja template to interface.py using dataclasses
jo47011 Nov 3, 2024
f51148c
fix: mypy & flake issues
jo47011 Nov 3, 2024
440da0e
fix: mypy & flake issues
jo47011 Nov 4, 2024
5a3e539
remove: auto open menu on-hover
jo47011 Nov 4, 2024
430aad7
fix: docs
jo47011 Nov 4, 2024
d09ed56
fix: use page name instead of path
jo47011 Nov 4, 2024
2956f34
fix: mypy issues
jo47011 Nov 4, 2024
da842f7
fix: docs reference page and add_menu_entry
jo47011 Nov 4, 2024
08e412d
remove: package-lock.json (sorry)
jo47011 Nov 4, 2024
8b84b07
fix: doc
jo47011 Nov 22, 2024
14d3982
fix: use fomantic css for new `activated` class
jo47011 Nov 22, 2024
8a96460
fix: use fomantic css for new `activated` class
jo47011 Nov 22, 2024
e36f320
fix: flake issue
jo47011 Nov 22, 2024
0c6b95d
fix: typo in type hint
jo47011 Nov 22, 2024
ab85ae3
fix: doc, remove is_active flag
jo47011 Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions example/ui_showcase.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,17 @@ async def do_magic(_v, _o) -> None:

#############################################################################################
# Overview page #
# Here we show the ImageMap and HideRows #
# Here we show the ImageMap, HideRows and sub menus. #
#############################################################################################

overview_page = web_server.page('overview', "Overview", menu_entry='Some Submenu', menu_icon='tachometer alternate',
menu_sub_label="Overview")
menu_sub_label="Overview", menu_sub_icon='tachometer alternate')

submenu_page2 = web_server.page('submenu2', "Nothing here", menu_entry='Some Submenu', menu_sub_icon='couch',
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")
Copy link
Owner

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.

Copy link
Contributor Author

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?

Copy link
Owner

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.

Copy link
Contributor Author

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:

image

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.

Copy link
Contributor Author

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.

Done.


# ImageMap supports all the different Buttons as items, as well as the special ImageMapLabel
# The optional fourth entry of each item is a list of WebPageItems (everything we have shown so far – even an ImageMap))
Expand Down
5 changes: 3 additions & 2 deletions shc/web/interface.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,8 +292,9 @@ async def _page_handler(self, request: aiohttp.web.Request) -> aiohttp.web.Respo
html_title = self.title_formatter(page.title)
template = jinja_env.get_template('page.htm')
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)
qroot_url=self.root_url, js_files=self._js_files, css_files=self._css_files,
server_token=id(self), html_title=html_title,
current_url=request.path)
return aiohttp.web.Response(body=body, content_type="text/html", charset='utf-8')

async def _ui_websocket_handler(self, request: aiohttp.web.Request) -> aiohttp.web.WebSocketResponse:
Expand Down
14 changes: 11 additions & 3 deletions shc/web/templates/base.htm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 %}"
Copy link
Owner

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().

Copy link
Contributor Author

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?

Copy link
Owner

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.

Copy link
Contributor Author

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.

href="{{ root_url }}/page/{{ link }}/">
{% if icon %}<i class="{{ icon }} icon"></i>{% endif %}
{{ label }}
</a>
{% else %}
<div class="mobile hidden ui dropdown item">
{% 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 %}">
Copy link
Contributor Author

@jo47011 jo47011 Nov 3, 2024 •
edited
Loading

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?

Copy link
Owner

@mhthies mhthies Nov 3, 2024 •
edited
Loading

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

{% if icon %}<i class="{{ icon }} icon"></i>{% endif %}
{{ label }}
<i class="dropdown icon"></i>
<div class="menu">
{% for sub_label, sub_icon, sub_link in link %}
<a class="item" href="{{ root_url }}/page/{{ sub_link }}/">
<a class="item {% if '/page/' ~ sub_link ~ '/' == current_url %}selected{% endif %}"
href="{{ root_url }}/page/{{ sub_link }}/">
{% if sub_icon %}<i class="{{ sub_icon }} icon"></i>{% endif %}
{{ sub_label }}
</a>
Expand Down
52 changes: 52 additions & 0 deletions test/test_web.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,65 @@ def test_main_menu(self) -> None:
submenu_entry = submenu.find_element(By.XPATH, './/a[contains(@class, "item")]')
self.assertFalse(submenu_entry.is_displayed())
submenu.click()
actions = ActionChains(self.driver)
actions.move_to_element(submenu).perform() # do a hoover

Copy link
Contributor Author

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.

Copy link
Owner

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.

Copy link
Contributor Author

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.

self.assertIn("Bar", submenu_entry.text)
self.assertTrue(submenu_entry.is_displayed())
submenu_entry_href = submenu_entry.get_attribute('href')
assert submenu_entry_href is not None
self.assertEqual(submenu_entry_href.strip(), "http://localhost:42080/page/another_page/")
submenu_entry.find_element(By.CSS_SELECTOR, 'i.bars.icon')

def test_main_menu_selection(self) -> None:
"""Test that the clicked menu item is selected thus highlighted."""
self.server.page('index', menu_entry="Home", menu_icon='home')
self.server.page('overview', menu_entry="Foo", menu_icon='info')

# add submenu
Copy link
Owner

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

self.server.page('submenu1', "Sub1", menu_entry='Some Submenu', menu_icon='bell',
menu_sub_label="Overview")
self.server.page('submenu2', "Sub2", menu_entry='Some Submenu',
menu_sub_label="Empty Submenu")
self.server.page('submenu3', "Sub3", menu_entry='Some Submenu',
menu_sub_label="Empty Submenu 2")

self.server_runner.start()
self.driver.get("http://localhost:42080")

# test on startup only 1st item is selected
container = self.driver.find_element(By.CSS_SELECTOR, '.pusher')
selected_menus = container.find_elements(By.CLASS_NAME, 'selected')
self.assertEqual(len(selected_menus), 1)
self.assertIn("Home", selected_menus[0].text)

# test after click on foo only the clicked item is selected
foo_link = container.find_element(By.CSS_SELECTOR, 'i.info.icon').find_element(By.XPATH, '..')
foo_link.click()

container = self.driver.find_element(By.CSS_SELECTOR, '.pusher')
selected_menus = container.find_elements(By.CLASS_NAME, 'selected')
self.assertEqual(len(selected_menus), 1)
self.assertIn("Foo", selected_menus[0].text)

# click top level submenu item 1st to open submenu
submenu = container.find_element(By.CSS_SELECTOR, 'i.bell.icon').find_element(By.XPATH, '..')
submenu.click()
actions = ActionChains(self.driver)
actions.move_to_element(submenu).perform() # do a hoover

# now select submenu item
submenu_entry = submenu.find_element(By.XPATH, './/a[contains(@class, "item")]')
submenu_entry.click()

# test after selecting a submenu both are selected the submenu item and the menu item
container = self.driver.find_element(By.CSS_SELECTOR, '.pusher')
selected_menus = container.find_elements(By.CLASS_NAME, 'selected')
self.assertEqual(len(selected_menus), 2)
self.assertIn("Some Submenu", selected_menus[0].text)
submenu_href: str = str(selected_menus[1].get_attribute("href"))
self.assertTrue(submenu_href.endswith("/page/submenu1/"))


class MonitoringTest(unittest.TestCase):
def setUp(self) -> None:
Expand Down
3 changes: 3 additions & 0 deletions web_ui_src/main.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ body.pushable>.pusher, body:not(.pushable) {
line-height: 36px;
}

.item.selected {
background: url("prism.png") !important;
}

/* ************************** *
* Semantic UI extensions *
Expand Down
2 changes: 1 addition & 1 deletion web_ui_src/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'});
Copy link
Contributor Author

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.

Copy link
Owner

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feature removed, see above.

$('.ui.sidebar').sidebar({transition: 'overlay'}).sidebar('attach events', '#mobile_item');
});