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

Visual timeline UI #4650

Merged
merged 46 commits into from
Oct 22, 2024
Merged

Conversation

cstns
Copy link
Contributor

@cstns cstns commented Oct 16, 2024

Description

Implemented the user interface described in #4304 for the history timeline

Related Issue(s)

part of #4304
has to be merged in #4647

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 cstns self-assigned this Oct 16, 2024
@cstns cstns marked this pull request as draft October 16, 2024 07:22
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.53%. Comparing base (d0df0d4) to head (5d9f210).
Report is 47 commits behind head on visual-timeline-of-version-history.

Additional details and impacted files
@@                         Coverage Diff                         @@
##           visual-timeline-of-version-history    #4650   +/-   ##
===================================================================
  Coverage                               78.53%   78.53%           
===================================================================
  Files                                     303      303           
  Lines                                   14398    14398           
  Branches                                 3285     3285           
===================================================================
  Hits                                    11307    11307           
  Misses                                   3091     3091           
Flag Coverage Δ
backend 78.53% <ø> (ø)

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.

cstns added 2 commits October 17, 2024 16:18
… panels and margin bottom to compensate for the hubspot chat
@cstns cstns marked this pull request as ready for review October 21, 2024 10:10
@cstns
Copy link
Contributor Author

cstns commented Oct 21, 2024

nb: the high number of files changed is due to directory changes made to reflect the new version history hierarchy. Moved files should have no code changes

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.

UX Feedback:

  • If I have the feature disabled in my Team Type, then it should default to the "Snapshots" view, not "Timeline"

  • We need some better visual feedback :hover of the toggle between Snapshots and Timeline. Currently only have a cursor change, hover background color should be applied.

  • I know the timeline is vertical, but three vertical dots implies kebab menu rather than "more", so let's go horizontal please.

Screenshot 2024-10-21 at 12 07 44
  • After Creating a Snapshot in the "Timeline" view, the new Snapshot doesn't display, I have to refresh, or navigate away and back and agian.

@joepavitt
Copy link
Contributor

I also seem to not be seeing any auto-snapshots in the timeline? I know we discussed how busy that made it, but thought we'd agreed to keep them in?

@cstns
Copy link
Contributor Author

cstns commented Oct 21, 2024

  • If I have the feature disabled in my Team Type, then it should default to the "Snapshots" view, not "Timeline"

That was my main thought but settled on let's showcase the timeline even though you don't have access to it. I agree that it would be an additional step to users without access, ill change it in a bit.

I know the timeline is vertical, but three vertical dots implies kebab menu rather than "more", so let's go horizontal please.

willdo!

After Creating a Snapshot in the "Timeline" view, the new Snapshot doesn't display, I have to refresh, or navigate away and back and agian.

I added the load more functionality last minute and apparently messed up the loading snapshots after creation

We need some better visual feedback :hover of the toggle between Snapshots and Timeline. Currently only have a cursor change, hover background color should be applied.

The load more functionality messed this up as well, the loading animation should have been present.

I also seem to not be seeing any auto-snapshots in the timeline? I know we discussed how busy that made it, but thought we'd agreed to keep them in?

I didn't find a way to disable them locally so left them at that. They appear to be working fine on my end albeit I still have no idea how to disable/enable them
image

@joepavitt
Copy link
Contributor

UI:

Screenshot 2024-10-21 at 14 58 27

So, newer auto-snapshots are showing, the older ones are not? @Steve-Mcl - is this expected?

API Response:

Screenshot 2024-10-21 at 14 58 01

@Steve-Mcl
Copy link
Contributor

Steve-Mcl commented Oct 21, 2024

So, newer auto-snapshots are showing, the older ones are not? @Steve-Mcl - is this expected?

Hard to say without seeing all data but FYI, the API returns paged results & I can see there is a next_cursor - perhaps its on the next page?

Are these on the pre-staging or local @joepavitt ?

Happy to do a call to investigate.

@cstns
Copy link
Contributor Author

cstns commented Oct 21, 2024

I'd like to join that call if it 's alright with you

@joepavitt
Copy link
Contributor

Just local, and further pages do not contain the auto-created Snapshots. Can you verify on any instances you may have locally yourself? My throat still playing up, so trying to minimise talking.

@Steve-Mcl
Copy link
Contributor

Just local, and further pages do not contain the auto-created Snapshots. Can you verify on any instances you may have locally yourself? My throat still playing up, so trying to minimise talking.

No worries.

I'm just finishing up a docs PR. Will pull this and call @cstns (about 15 mins ok ?)

@Steve-Mcl
Copy link
Contributor

some observations noted after pulling this branch - just getting them written up so they are not forgotten.

  1. If my team does not have the visual timeline, should it not default to original snapshot view? (a little tedious having to always switch)
    • image
  2. Pictograms used are not snapshot (happy to concede this is just a copy paste of current dialog :) )
    • image
    • image

@Yndira-E
Currently, FF Cloud Production (and this PR) are using node_catalog_red.png for snapshots info dialogs.
Should they be using snapshot_red.png?


.

@Yndira-E
Copy link
Contributor

Hi @Steve-Mcl

Should they be using snapshot_red.png?

For snapshots, absolutely. However, for the timeline, we might need something new for both the dialog and the empty state. For now, I think we can use the same snapshot image for both dialogues and update the timeline one when we have it.

I've already created the art request: #4668

@cstns
Copy link
Contributor Author

cstns commented Oct 21, 2024

...we might need something new for both the dialog and the empty state.

We only need new icons for the feature unavailable state and dialog, because users that can access the feature on any instance will always have the instance created event, so the list will never be empty.

@cstns
Copy link
Contributor Author

cstns commented Oct 21, 2024

  1. If my team does not have the visual timeline, should it not default to original snapshot view? (a little tedious having to always switch)

I added this in a later edit, this should be the case now

@Steve-Mcl
Copy link
Contributor

@joepavitt

it is unnerving how quickly we parse things out of our memory!

Spend 20 mins debugging with @cstns then something popped into my head...

From the PR: #4509

Enable snapshot audit logging for "auto snapshots"

We weren't capturing an audit entry prior to ...

image

In an early version of this code, I scanned the snapshots and interleaved them with the audit entries, however this was riddled with edge cases and subtleties so instead, I did what should have been done from the start - audit log the Auto Snapshots.

Moving forward, you will see all snapshots.

@joepavitt joepavitt self-requested a review October 22, 2024 09:35
@cstns cstns merged commit 376a38a into visual-timeline-of-version-history Oct 22, 2024
14 checks passed
@cstns cstns deleted the visual-timeline-ui branch October 22, 2024 09:37
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.

4 participants