-
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
Changes from 12 commits
130250e
8e90389
2a309c4
67d03df
97ed155
1d1d5c5
85f6b4a
5f68432
2125048
6c5d216
f51148c
440da0e
5a3e539
430aad7
d09ed56
2956f34
da842f7
08e412d
8b84b07
14d3982
8a96460
e36f320
0c6b95d
ab85ae3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
import os | ||
import pathlib | ||
import weakref | ||
from dataclasses import dataclass, field | ||
from json import JSONDecodeError | ||
from typing import Dict, Iterable, Union, List, Set, Any, Optional, Tuple, Generic, Type, Callable | ||
|
||
|
@@ -42,6 +43,26 @@ | |
LastWillT = Tuple["WebApiObject[T]", T] | ||
|
||
|
||
@dataclass | ||
class MenuEntrySpec(Generic[T]): | ||
"""Specification of one menu entry within the :class:`WebServer`""" | ||
# The name of the page (link target) | ||
page_name: Optional[str] = None | ||
# The label of the entry in the main menu | ||
label: Optional[str] = None | ||
# If given, the menu entry is prepended with the named icon | ||
icon: Optional[str] = None | ||
# Flag whether this menu item is active | ||
is_active: bool = False | ||
|
||
|
||
@dataclass | ||
class SubMenuEntrySpec(MenuEntrySpec): | ||
"""Specification of a sub menu entry within the :class:`WebServer`""" | ||
# List of submenus if any | ||
submenus: List[MenuEntrySpec] = field(default_factory=list) | ||
|
||
|
||
class WebServer(AbstractInterface): | ||
""" | ||
A SHC interface to provide the web user interface and a REST+websocket API for interacting with Connectable objects. | ||
|
@@ -81,14 +102,14 @@ def __init__(self, host: str, port: int, index_name: Optional[str] = None, root_ | |
self._associated_tasks: weakref.WeakSet[asyncio.Task] = weakref.WeakSet() | ||
# last will (object, value) per API websocket client (if set) | ||
self._api_ws_last_will: Dict[aiohttp.web.WebSocketResponse, LastWillT] = {} | ||
# data structure of the user interface's main menu | ||
# using class `MenuEntrySpec` and `SubMenuEntrySpec` as data structure for the user interface's main menu | ||
# The structure looks as follows: | ||
# [('Label', 'icon', 'page_name'), | ||
# ('Submenu label', None, [ | ||
# ('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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. you're so right. Fixed. |
||
# ]), | ||
# ...] | ||
self.ui_menu_entries: List[Tuple[str, Optional[str], Union[str, List[Tuple[str, Optional[str], str]]]]] = [] | ||
self.ui_menu_entries: List[MenuEntrySpec] = [] | ||
# List of all static js URLs to be included in the user interface pages | ||
self._js_files = [ | ||
"/static/pack/main.js", | ||
|
@@ -207,21 +228,26 @@ def add_menu_entry(self, page_name: str, label: str, icon: Optional[str] = None, | |
:raises ValueError: If there is already a menu entry with the same label (or a submenu entry with the same two | ||
labels) | ||
""" | ||
existing_entry = next((e for e in self.ui_menu_entries if e[0] == label), None) | ||
existing_entry: Optional[MenuEntrySpec] = next((e for e in self.ui_menu_entries if e.label == label), None) | ||
if not sub_label: | ||
if existing_entry: | ||
raise ValueError("UI main menu entry with label {} exists already. Contents: {}" | ||
.format(label, existing_entry[2])) | ||
self.ui_menu_entries.append((label, icon, page_name)) | ||
.format(label, existing_entry.page_name)) | ||
self.ui_menu_entries.append(MenuEntrySpec(label=label, icon=icon, page_name=page_name)) | ||
|
||
elif existing_entry: | ||
if not isinstance(existing_entry[2], list): | ||
if not isinstance(existing_entry, SubMenuEntrySpec): | ||
raise ValueError("Existing UI main menu entry with label {} is not a submenu but a link to page {}" | ||
.format(label, existing_entry[2])) | ||
existing_entry[2].append((sub_label, sub_icon, page_name)) | ||
|
||
.format(label, existing_entry.page_name)) | ||
existing_entry.submenus.append(MenuEntrySpec(label=sub_label, icon=sub_icon, page_name=page_name)) | ||
else: | ||
self.ui_menu_entries.append((label, icon, [(sub_label, sub_icon, page_name)])) | ||
self.ui_menu_entries.append( | ||
SubMenuEntrySpec( | ||
label=label, | ||
icon=icon, | ||
submenus=[MenuEntrySpec(label=sub_label, icon=sub_icon, page_name=page_name)], | ||
) | ||
) | ||
|
||
def api(self, type_: Type, name: str) -> "WebApiObject": | ||
""" | ||
|
@@ -291,11 +317,35 @@ 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) | ||
self._mark_active_menu_items(request.path) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should pass the page name ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are so right. Thanks 👍 |
||
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, | ||
SubMenuEntrySpec=SubMenuEntrySpec, | ||
) | ||
return aiohttp.web.Response(body=body, content_type="text/html", charset='utf-8') | ||
|
||
def _mark_active_menu_items(self, path: str): | ||
"""Set menu items is_active flag if current page matches page_name/target link.""" | ||
page_name = path.lstrip("/page/").strip("/") | ||
for item in self.ui_menu_entries: | ||
if isinstance(item, SubMenuEntrySpec): | ||
item.is_active = False | ||
for sub_item in item.submenus: | ||
if page_name == sub_item.page_name: | ||
item.is_active = True | ||
sub_item.is_active = True | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. I really don't like that this method changes the This may even result in concurrency issues, considering, that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, understood & fixed. |
||
|
||
async def _ui_websocket_handler(self, request: aiohttp.web.Request) -> aiohttp.web.WebSocketResponse: | ||
ws = aiohttp.web.WebSocketResponse() | ||
await ws.prepare(request) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -141,13 +141,67 @@ 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 | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strange, with the open on hover in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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, 'activated') | ||
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, 'activated') | ||
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') | ||
self.assertEqual(len(selected_menus), 1) | ||
selected_menus = container.find_elements(By.CLASS_NAME, 'activated') | ||
self.assertIn("Some Submenu", selected_menus[0].text) | ||
selected_menus = container.find_elements(By.CLASS_NAME, 'selected') | ||
self.assertEqual(len(selected_menus), 1) | ||
submenu_href: str = str(selected_menus[0].get_attribute("href")) | ||
self.assertTrue(submenu_href.endswith("/page/submenu1/")) | ||
|
||
|
||
class MonitoringTest(unittest.TestCase): | ||
def setUp(self) -> None: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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 commentThe 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'); | ||
}); |
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(...)
andadd_menu_entry(...)
is kind of confusing and not really obvious, e.g. that inadd_menu_entry()
the existing menu_entry is found by the label. Maybe we should align the attribute names in both methods.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 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.
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.
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.
Done.