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

Use default behavior for platform wide anchors (part I) #4834

Merged
merged 22 commits into from
Dec 6, 2024

Conversation

cstns
Copy link
Contributor

@cstns cstns commented Nov 27, 2024

Description

First iteration to correct platform wide link interaction to allow mouse+keyboard combinations.

  • Fixes inconsistencies when opening the immersive editor from different places using key + mouse click
    • for example the applications instances open editor button was opening the regular editor when ctrl+clicking even though the immersive one was available
  • added middle mouse click support
  • restores default anchor behavior to links on the page header (except team selector), applications overview page, application instances and application devices pages
  • links can be opened via cmd/ctrl + click or middle mouse button click
  • had to alter the datatable components to trickle-up the click event in order to be able to decide in we open in a new tab or current one
  • in order to identify middle mouse button clicks I had to replace the @click event handler to @mouseup which includes it
  • simplified the open editor method while fixing aforementioned bugs
  • used the vue router where possible to preserve navigation integrity
  • added cmd/ctrl+click and middle mouse button interactivity to the open dashboard

Related Issue(s)

closes #4837
closes #4838

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

cstns added 15 commits November 26, 2024 10:51
…n and emitting events, use the navigateTo composable helper to handle ff-button navigation
…tances page (opening another tab while also navigating)

- switch from click to mouseup events to capture middle mouse button click
- simplified the open editor method while fixing aforementioned bugs
- used the vue router where possible to preserve navigation integrity
@cstns cstns self-assigned this Nov 27, 2024
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.74%. Comparing base (9bb28ab) to head (d57b40b).
Report is 23 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4834   +/-   ##
=======================================
  Coverage   78.74%   78.74%           
=======================================
  Files         323      323           
  Lines       15221    15221           
  Branches     3496     3496           
=======================================
  Hits        11986    11986           
  Misses       3235     3235           
Flag Coverage Δ
backend 78.74% <ø> (ø)

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.

…r type from the dashboard link to correctly handle routing
Copy link
Contributor

@joepavitt joepavitt left a comment

Choose a reason for hiding this comment

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

Mixins

I will comment in saying I really don't like our move towards a mixin approach as it makes the traceability of our in-component variables incredibly difficult and the code general much harder to navigate.

However, I will not block this work for that reason, but just ask, in the future, that we consider for developer-friendly approaches in terms of readibility.

Sidebar Rendering

Also with the open-in-new-tab functionality introduced here, it does also stress how poorly reactive our sidebar is and it's not rendering whenever I open most of the links here.

General Comments

I've also made a few other comments within the changes directly, which have questions I'd like answered

Comment on lines +10 to +11
@click.stop.prevent
@mouseup.stop.prevent
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these 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.

to prevent the click event from bubbling up/down. In the case of the dashboard link being nested inside another anchor/link/button, this prevents opening both parent anchor and dashboard link when clicking (or any other click + keyboard combination) directly on the dashboard link element

kind="secondary"
data-action="open-editor"
:disabled="buttonDisabled"
class="whitespace-nowrap"
@click.stop="openEditor"
:emit-instead-of-navigate="true"
@select="openEditor"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why @select?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

another way of preventing click events from bubbling up/down. This is specific to the editor link button, having to open different urls based on the type of click/combination acted upon. Emitting an event instead of propagating the click event when clicked gets rid of a lot of headaches and changes when dealing with event propagation in parent components.

I have to admit, 'select' is not the best choice of name for the event being emitted when clicked

@cstns
Copy link
Contributor Author

cstns commented Dec 5, 2024

I will comment in saying I really don't like our move towards a mixin approach as it makes the traceability of our in-component variables incredibly difficult and the code general much harder to navigate.

However, I will not block this work for that reason, but just ask, in the future, that we consider for developer-friendly approaches in terms of readibility.

Slowly but surely I'm moving away from the mixin approach, but it will take some time in doing so. The long term solution is to extract all generic stateless functionality into composables and delegate the rest into appropriate stores/services

Also with the open-in-new-tab functionality introduced here, it does also stress how poorly reactive our sidebar is and it's not rendering whenever I open most of the links here.

This is very unfortunate side effect which is made even more obvious by this feature and has a greater impact than the sidebar itself. Because we don't have a safeguard in place and render views before we have all mandatory data for the ui to function properly, there are scenarios where the team selector isn't present and depending on the tab you're opening, even elements on the page misbehave.

I am aware of them and will address them in the upcoming period.

@cstns cstns requested a review from joepavitt December 5, 2024 09:39
@joepavitt joepavitt merged commit a67c117 into main Dec 6, 2024
13 checks passed
@joepavitt joepavitt deleted the use-default-behavior-for-platform-wide-anchors branch December 6, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants