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

feat(ui): flatten UI when elevation is not necessary (#2317) by @floutchito #2317

Merged
merged 12 commits into from
Mar 23, 2022

Conversation

floutchito
Copy link
Contributor

@floutchito floutchito commented Mar 13, 2022

I guys.

I'm using this piece of code every day via home assistant, and I love it !

However, I noticed minor possible enhancements that I propose to share with this PR.
Comments and remarks are welcome.

Here's what is modified :

  • A "Home Assistant mode" for UI navigation (using tabs in the top app bar instead of a navigation drawer, to prevent dual drawer when integrated in Home Assistant)
  • Flatten UI when elevation is not necessary. There are places in the app where logical containers do not need elevation, this PR proposes to flatten these for a better readability

image

  • There is also a new UI section in the settings page where I added the new option "Use tabs for navitation" (aka "HA mode"), and moved the dark mode toggle that was previously in the nav drawer

image

  • Refactored controller statistics panel and actions menu

Statistics panel now shows all possible stats grayscaled when empty, and blue / red colors are applied when a value is set.
This makes the panel size, and the stats positions predictable over time, whereas it does not visually take more place.

I think it is useful when someone is quickly searching for a specific stat.
Stats are also grouped by logical sections: MESSAGES, COMMANDS, COMMUNICATION.

image
image

Add a UI section in settings
Move dark mode switch to UI Settings section
Add UI setting to replace Navigation Drawer with tabs, useful when app is embedded in another one, typically Home Assistant integration
Remove cards nesting when unnecessary
Flatten elevation when unnecessary
Use buttons instead of ExpansionPanels for actions and statistics on ControlPanel
Refactored StatisticsCard.vue
@floutchito floutchito changed the title UI flattened proposal feat(ui) UI flattened proposal Mar 13, 2022
@floutchito floutchito changed the title feat(ui) UI flattened proposal feat(ui): UI flattened proposal Mar 13, 2022
@floutchito floutchito changed the title feat(ui): UI flattened proposal feat(ui): flatten UI when unnecessary Mar 13, 2022
@floutchito floutchito changed the title feat(ui): flatten UI when unnecessary feat(ui): flatten UI when necessary Mar 13, 2022
@floutchito floutchito changed the title feat(ui): flatten UI when necessary feat(ui): flatten UI when elevation is not necessary Mar 13, 2022
@floutchito
Copy link
Contributor Author

This is a Draft, I'm searching for an easy way to use the HA zwave-js-server to continue this work on node views, but it seems I can't link the dev server to an existing prod instance embedded in HA.

My HA instance and its ZWJS2MQTT plugin are dockerized by Supervisor.

Do some of you guys know a way to do so ?

@coveralls
Copy link

coveralls commented Mar 14, 2022

Pull Request Test Coverage Report for Build 2025337885

  • 0 of 28 (0.0%) changed or added relevant lines in 1 file are covered.
  • 184 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.04%) to 27.023%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/store/mutations.js 0 28 0.0%
Files with Coverage Reduction New Missed Lines %
src/store/mutations.js 1 0%
config/store.ts 8 0%
lib/ZwaveClient.ts 15 0%
lib/Gateway.ts 160 17.4%
Totals Coverage Status
Change from base Build 1979446626: -0.04%
Covered Lines: 3642
Relevant Lines: 14210

💛 - Coveralls

@robertsLando
Copy link
Member

Hiu @floutchito ! Really thanks for your PR, love it! I agree the UI can be improved a lot, like you can see I'm not an UI fun, I'm more a backend guy 😆

I'm searching for an easy way to use the HA zwave-js-server to continue this work on node views, but it seems I can't link the dev server to an existing prod instance embedded in HA.

About your question, you should get the ip address of the docker container that is running the webserver and set it here:

https://github.com/zwave-js/zwavejs2mqtt/blob/master/src/apis/ConfigApis.js#L10

Let me know if you need something else! I will give a try to your PR soon

@robertsLando
Copy link
Member

robertsLando commented Mar 14, 2022

I tested your PR. Everything looks good, I like all the style you used. The only thing that seems not working is controller stats, when I enable it nothing is shown

Also when Show tabs for navigation option is enabled there is an overflow here:

Schermata da 2022-03-14 10-16-04

src/App.vue Outdated
@@ -845,3 +860,9 @@ export default {
},
}
</script>

<style scoped>
.smaller-min-width-tabs {
Copy link
Member

Choose a reason for hiding this comment

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

In order to make this working with scoped you should use >>> operator

.v-tabs >>> .smaller-min-width-tabs

@floutchito
Copy link
Contributor Author

Hiu @floutchito ! Really thanks for your PR, love it! I agree the UI can be improved a lot, like you can see I'm not an UI fun, I'm more a backend guy 😆

I'm searching for an easy way to use the HA zwave-js-server to continue this work on node views, but it seems I can't link the dev server to an existing prod instance embedded in HA.

About your question, you should get the ip address of the docker container that is running the webserver and set it here:

https://github.com/zwave-js/zwavejs2mqtt/blob/master/src/apis/ConfigApis.js#L10

Let me know if you need something else! I will give a try to your PR soon

Thanks !

I'll give a try at setting the IP of my prod server.
Therefore, I would be able to test statistics and node views, I must say that I wasn't able to test it with real data.

Will work on this at night.

@robertsLando
Copy link
Member

Will work on this at night.

Perfect! Thanks :)

robertsLando and others added 7 commits March 18, 2022 07:21
…space

Removed wrapping VCard
Positioned file browser sticky to the top of the view
Moved file selection VSpeedDial to file browser view

Positioned file content actions sticky to the bottom of the file content view

Vertically centered placeholder text and progress
Small typographic styling
Pulled Ping / Advanced node actions out of NodeDetails
Using vertical tabs
Unwrapped manual instructions from tabs
Flattened values expansion panels
@floutchito
Copy link
Contributor Author

Hi there.

I finally managed to use my HA `zwave-js-server` linked to a dev instance of `zwjs2mqtt` !

I wasn't able to connect to the addon's IP, 'cause my HA is on a specific host, and addons are isolated in a specific Docker network.
I tryied to connect from HA's external URL, but I got trouble with CORS issues.
Even after I bypassed the CORS headers (with CORS anywhere or MITMProxy), I had 401 Unauthorized errors, that was thrown by the addon's internal nginx.

I had to forward the internal port 44290 from inside of the hassio Docker network, through socat and expose the result to the host. After that I was able to bypass the nginx-level security built-in the HA add-on.

Here's a docker-compose example :

version: '3.7'

services:
      
  zwave-server-forwarder:
    image: "alpine/socat"
    command: "tcp4-listen:8099,fork,reuseaddr,su=nobody tcp-connect:172.XX.YY.ZZ:44920"
    networks:
      hassio:
    ports:
      - "8099:8099"
      
networks:
  hassio:
    external: true

And after a bit of work, I ended with the following result :

Working StatisticsCard

image

Reworked Store view

I tried to use the available space instead of limiting the file browser to a max of 800px.
I tested it in many ways, and I couldn't find a situation where I missed controls or view.

Screenshot 2022-03-20 at 14-56-33 ZWave To MQTT

Reworked inner Nodes view

Here, I tried to maximize the number of infomations available at the same time.
To do so, I switched the sections to vertical tabs, in order to always see the different available sections.
I also choosed to pull Device ID and the PING and ADVANCED actions buttons out of NodeDetails view.
I thought it should be useful to access these generic actions from anywhere in the opened node view.
These are obviously opinionated choices, let me know if it makes sense to you.

Screenshot 2022-03-20 at 14-53-08 ZWave To MQTT

I also flattened the expansion panels to match the Settings view styles
Screenshot 2022-03-20 at 14-57-25 ZWave To MQTT

Fixed The small overflow in AppBar

I didn't modified the graph view, and I do not use Scenes, so I can't see if there are view that I can work on.

Let me know if there are thing that needs to be rolledback or modified.

@floutchito floutchito marked this pull request as ready for review March 20, 2022 15:20
Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

@floutchito Firstly thanks for this PR, just love it!

I tested your changes and everything looks good! Just two things:

  1. On smaller screen I think it makes sense to have node tabs (in node details) horizontally instead of vertically, could you change that?
  2. The controller stats button seems like it's not clickable when stats are hidden, I would change the disabled color to something more user friendly so the users can detect it as something clickable

The rest LGTM :)

@floutchito
Copy link
Contributor Author

floutchito commented Mar 22, 2022

Hi @robertsLando,

1- On smaller screen I think it makes sense to have node tabs (in node details) horizontally instead of vertically, could you change that?

Done. I had to align left the AssociationGroups table to let users understand the structure on smaller screen.

2- The controller stats button seems like it's not clickable when stats are hidden, I would change the disabled color to something more user friendly so the users can detect it as something clickable

Done. I also added a toggle for each individual node to show / hide statistics.

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @floutchito

@robertsLando robertsLando changed the title feat(ui): flatten UI when elevation is not necessary feat(ui): flatten UI when elevation is not necessary (#2317) by @floutchito Mar 23, 2022
@robertsLando robertsLando merged commit 97ec643 into zwave-js:master Mar 23, 2022
@AlanGreene
Copy link

AlanGreene commented Mar 23, 2022

@floutchito I just updated to release 6.6.0 in my Home Assistant instance and noticed the Dark Mode setting doesn't apply when I first load the page. To enable dark mode I must first navigate to settings, then to any other Z-Wave JS page. It remains enabled as long as I stay within Z-Wave JS. As soon as I navigate away to another page in HA and come back to Z-Wave JS it's reverted to displaying in light mode again.

Also the Save button in settings isn't reachable in the HA Android app, it's rendered off the right edge of the screen.

Let me know if there's any particular info you need to help debug, happy to provide any logs / network traces, versions etc. that may be of use.

@floutchito
Copy link
Contributor Author

@floutchito I just updated to release 6.6.0 in my Home Assistant instance and noticed the Dark Mode setting doesn't apply when I first load the page. To enable dark mode I must first navigate to settings, then to any other Z-Wave JS page. It remains enabled as long as I stay within Z-Wave JS. As soon as I navigate away to another page in HA and come back to Z-Wave JS it's reverted to displaying in light mode again.

Also the Save button in settings isn't reachable in the HA Android app, it's rendered off the right edge of the screen.

Let me know if there's any particular info you need to help debug, happy to provide any logs / network traces, versions etc. that may be of use.

Hello,

I can reproduce this bug. I'll look at it as soon as possible.

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.

5 participants