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

Broker Topic Hierarchy UI #4790

Merged
merged 29 commits into from
Nov 20, 2024
Merged

Broker Topic Hierarchy UI #4790

merged 29 commits into from
Nov 20, 2024

Conversation

cstns
Copy link
Contributor

@cstns cstns commented Nov 18, 2024

Description

  • add the mqtt broker page under the unified-namespace namespace
  • add the new topic hierarchy page under the same namespace
  • addressed the permissions composable reactivity problem which bit me sooner than expected

Was not able to re-use the existing accordion component because it was toggling the visibility of it's children which caused problems when rendering high numbers of topics. Ended up creating simpler a TopicSegment component which improved DOM rendering due to it not inserting the node in the dom only when acted upon.

Was able to render 1500 topic entries with 100 nested topics with ease.

Related Issue(s)

closes #4752

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

@hardillb
Copy link
Contributor

Does this want to target the other PR branch first?

@cstns
Copy link
Contributor Author

cstns commented Nov 18, 2024

yes, my bad! Thank you for pointing it out!

I'll need a couple of hours to solve the e2e tests though

@cstns cstns changed the base branch from main to track-used-topics November 18, 2024 11:15
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.72%. Comparing base (5475a81) to head (c27a4f5).
Report is 30 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4790   +/-   ##
=======================================
  Coverage   78.72%   78.72%           
=======================================
  Files         314      314           
  Lines       15053    15053           
  Branches     3457     3457           
=======================================
  Hits        11851    11851           
  Misses       3202     3202           
Flag Coverage Δ
backend 78.72% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@hardillb
Copy link
Contributor

Totally first impressions, just thinking out loud

  • I think the entry on the left should still be "Broker" not "UNS"
  • Need to decide what order the Clients, Hierarchy tabs need to be in
  • Need to think about the topic count on each row, showing 1 for test/ feels wrong when bill/ has 2

image

Otherwise looks good

@hardillb
Copy link
Contributor

Just asking to see how hard it would be, to remember expanded levels (only for while the page is currently open).

e.g. currently if I fully expand a topic tree, but then collapse the first element, when I re-expand that element again all the children have been collapsed, might be useful to be able to keep state for the lower elements

@cstns cstns self-assigned this Nov 18, 2024
@hardillb
Copy link
Contributor

The counts and the order we can iterate on afterwards

@cstns
Copy link
Contributor Author

cstns commented Nov 19, 2024

We are also restricting the use of the mqtt broker for self-hosted installations, are we keeping it in place this release?

Base automatically changed from track-used-topics to main November 19, 2024 11:34
@joepavitt
Copy link
Contributor

@cstns you have a merge conflict

# Conflicts:
#	frontend/src/pages/team/UNS/Clients/index.vue
Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

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

Tidies up some language in the UI

In terms of usability of the Topic Hierarchy - the key missing piece is it's actually quite hard to then get the full topic being used in a way I can copy/paste elsewhere.

Here I have published to the topic p1/line/factory/foo/p2/foo - and whilst I can see that in the hierarchy, if I want to use that value elsewhere, I have to still manually type it out.
A quick win would be to add a 'copy topic' button on the 'leaf' nodes of the tree that copies the fully formed topic string to my clipboard.

image

frontend/src/pages/team/UNS/index.vue Outdated Show resolved Hide resolved
frontend/src/pages/team/UNS/index.vue Outdated Show resolved Hide resolved
frontend/src/pages/team/routes.js Outdated Show resolved Hide resolved
cstns and others added 3 commits November 20, 2024 12:52
Co-authored-by: Nick O'Leary <nick.oleary@gmail.com>
Co-authored-by: Nick O'Leary <nick.oleary@gmail.com>
Co-authored-by: Nick O'Leary <nick.oleary@gmail.com>
@cstns
Copy link
Contributor Author

cstns commented Nov 20, 2024

In terms of usability of the Topic Hierarchy - the key missing piece is it's actually quite hard to then get the full topic being used in a way I can copy/paste elsewhere.

I wouldn't call it a missing piece, I don't remember it being mentioned before and it wasn't part of the requirements. It would be a nice addition but not in the format that I'm parsing the API response at the moment.

It can be easily implemented when tackling the topics count and remembering which topics were opened features @hardillb requested

…-name-space

# Conflicts:
#	frontend/src/pages/team/UNS/index.vue
@knolleary
Copy link
Member

I wouldn't call it a missing piece, I don't remember it being mentioned before and it wasn't part of the requirements.

To clarify - I acknowledge this was not in the original design, so I'm not say you missed anything that had been previously suggested.

I will raise a separate item for it as we don't need to block this for release - however I strongly feel we should iterate on it quickly.

@knolleary knolleary merged commit 8d89832 into main Nov 20, 2024
13 checks passed
@knolleary knolleary deleted the unified-name-space branch November 20, 2024 13:55
@cstns cstns mentioned this pull request Nov 22, 2024
10 tasks
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.

Build UI for displaying Broker Topic Hierarchy
4 participants