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

Simplify timelines UI/reuse UI elements #2629

Merged
merged 49 commits into from
Feb 23, 2023
Merged

Simplify timelines UI/reuse UI elements #2629

merged 49 commits into from
Feb 23, 2023

Conversation

tillprochaska
Copy link
Contributor

@tillprochaska tillprochaska commented Oct 19, 2022

See #2673 for context.

Scope of this PR

The test cases in the top-level test suite give a good impression of the main features:

PASS src/components/Timeline2/Timeline.test.tsx (7.005 s)
  ✓ sorts and filters items by temporal start (115 ms)
  ✓ allows creating new items (2015 ms)
  ✓ does not allow creating new items if not writeable (35 ms)
  ✓ allows removing entities (49 ms)
  ✓ does not allow removing entities if not writeable (49 ms)
  Sidebar
    ✓ allows changing entity color (120 ms)
    ✓ does not allow changing entity color if not writeable (35 ms)
    ✓ allows changing entity properties (190 ms)
    ✓ does not allow changing entity properties if not writeable (47 ms)
    ✓ allows changing entity-type properties (335 ms)
  Empty state
    ✓ displays a message (8 ms)
    ✓ allows creating new items (45 ms)
    ✓ does not allow creating new items if not writeable (2 ms)

Timelines will eventually have two different views (or renderers), a list view and a chart view. This PR mainly deals with refreshing the list view. However, I have already implemented a first version of the chart view (in src/components/Timeline2/TimelineChart). The chart view is not exposed in the UI right now.

In an ideal world, the chart view should be in a different PR. That would’ve resulted in more work (or splitting out this part into a separate PR and potentially losing part of the Git history).

I’d suggest that you either ignore the TimelineChart directory when reviewing this PR (instead reviewing it later, when we actually enable this view in the UI) or that you review what’s there now already. If you decide to do the latter, you can manually enable the chart view by adding a renderer="chart" property here.

How to review

I’d recommend reviewing in the following order:

  1. Take a look at the types in src/components/Timeline2/types.ts and the classes (TimelineItem, ImpreciseDate) in src/components/Timeline2/util.ts.
  2. src/screens/TimelineScreen/TimelineScreen.jsx
  3. src/components/Timeline2/Timeline.tsx and src/components/Timeline2/Timeline.test.tsx
  4. Then take a look at any components referenced in the Timeline component as well as respective tests.
  5. Anything else

I try to keep my git history clean and often add relevant implementation considerations to the commit message body. In case you’re wondering about something specific, it may make sense to click "View file" on GitHub and then "Blame" to find commits that have changed a specific file.

Also read through the implementation notes below.

Implementation notes

While this PR replaces most of the existing timelines implementation, I have kept the old implementation around while developing and have selectively used bits and pieces of the existing implementation. That is why there is a src/components/Timeline directory with the old implementation and a src/components/Timeline2 directory with the new implementation.

I will remove the old implementation before we merge this PR and rename the Timeline2 directory. This will remove about 1500 LOC, so the net-lines added in this PR will be closer to ~2500 rather than the ~4000 this PR has at the moment. Keep in mind that a big part of the lines added is for test coverage (this feature wasn’t tested before).

Apart from that, I have partially re-implemented the existing entity sidebar (that is already used in network diagrams) with the goal of porting it to TypeScript and refactoring it to make it easier to test. Right now, there exist two implementations of the same UI:

  • Old implementation used for network diagrams in src/react-ftm/components/NetworkDiagram/toolbox/EntityViewer.tsx
  • New implementation used in timelines in src/components/Timeline2/EntityViewer2.tsx

The mid-term goal is to replace the old implementation used in network diagrams. In order to keep the scope of this PR smaller, and to keep the risk of implementing bugs in network diagrams low, I haven’t done this in this PR.

To do

Before review:

  • Remove entities from timeline
  • Fix entity suggestions in entity viewer sidebar
  • Fix edge selects in create dialog
  • Improve keyboard navigation
    • Up/down arrows to select prev/next item
    • Clicking an item should set keyboard focus to it
    • Pressing escape should close entity sidebar
  • Use table-like layout (two columns for dates)
  • Rebase and reenable type checking in tests where we use window.crypto
  • Fix bug: Add entity and delete directly fails
  • Add proper translations
  • Add read-only mode (depending on permissions)

Before merging:

  • Delete Timeline directory and rename Timeline2 directory
  • Remove unrelated changes (see review comment)
  • Remove script to populate timelines during development

Potential enhancements:

  • Add status indicator ("Saved"/"Saving…"/"Error"/"Offline")
  • Display items without temporal extent separately
  • Improve ranking/order of properties in entity viewer, always display temporal properties first?
  • Add empty state
  • Add loading state
  • Format dates
  • Add tooltip to remove icon button
  • Use TimelineItem in EntityViewer
  • Don’t let people add items without at least one date field filled out
  • Move "Add item" button to the fixed entity set bar?
  • Add indicator for optional fields when creating a timeline item
  • Sort list by start or end date (asc or desc)
  • Add optional description field to create form / show description in list

Screenshots

Screen Shot 2023-01-11 at 16 49 56

Screen Shot 2023-01-11 at 16 48 29

Screen Shot 2023-01-11 at 16 52 22

@tillprochaska tillprochaska changed the base branch from main to develop October 19, 2022 16:57
@tillprochaska tillprochaska force-pushed the till/timelines branch 6 times, most recently from eb7b7b5 to 8782f45 Compare October 24, 2022 16:26
@tillprochaska tillprochaska changed the title Till/timelines Simplify timelines UI/reuse UI elements Nov 4, 2022
@tillprochaska tillprochaska mentioned this pull request Nov 4, 2022
27 tasks
@tillprochaska tillprochaska self-assigned this Nov 7, 2022
@tillprochaska tillprochaska force-pushed the till/timelines branch 4 times, most recently from 51ed7bf to f75a753 Compare November 11, 2022 10:22
@Rosencrantz Rosencrantz added this to the 3.14.0 milestone Nov 18, 2022
@tillprochaska tillprochaska force-pushed the till/timelines branch 4 times, most recently from 2a24a87 to caf8130 Compare December 27, 2022 16:15
@tillprochaska tillprochaska force-pushed the till/timelines branch 5 times, most recently from a15fa05 to e1905ca Compare December 29, 2022 20:04
@tillprochaska tillprochaska force-pushed the till/timelines branch 3 times, most recently from d5f9b50 to d8af93a Compare January 6, 2023 16:04
This copies the property values that are present in the old and the new schema.
`TimelineItem` encapsulates logic commonly required when displaying entities in a timeline, e.g. determining which color to use or what the earliest and latest date the entity covers are. This is in preparation for a new chart view, to ensure common display logic can be shared with the existing list view.
This allows sharing the logic across list and chart renderers.
This isn't enabled/visible to users at the moment.

* Supports displaying timeline items in a Gantt-chart like view.
* Supports single-day and multi-day items.
* Supports different degrees of precision (e.g. `2022`, `2022-01`, `2022-01-15`).
* Right now, there's no way to change the scale/resolution of the chart (day/week/month/quarter/year view).

I've added a new dependency, `date-fns`, to work with date intervals. We already use a date utility library (Moment.js), however, the project authors recommend not using Moment.js for new projects and Moment.js doesn't have out-of-the-box support for what I needed (e.g. calculating all the months, quarters, etc. within an interval). `date-fns` is a very modular library and allows us to use only what we need, so this adds less than 1kB to the bundle size.
… so they stay visible even if the timeline item is wider than the viewport
This makes the component code less complex and allows us to reuse logic in the future.
Initially, I kept types a little more relaxed, allowing component consumers to omit certain event handlers. This is handy especially in tests. For example, a test case may not be dependent on passing a real `onChange` handler to a component. When rendering the component in that test, it's convenient to simply omit the `onChange` property rather than passing a no-op event handler.

However, this requires at least some conditional logic in the components. This logic exists only to make writing tests easier. In order to keep the components and prop types as simple as possible, I've marked all props required that are only option for testing purposes. In order to avoid cluttering tests with the same boilerplate props over and over again, I've simply created `defaultProps` objects that can be spread into the components.
This fixes a bug where creating and removing an entity (without reloading the page between both actions) would result in an error. When a user creates a new entity in a timeline, an ID for the entity is auto-generated. When creating a new entity, Aleph appends a HMAC signature (based on dataset ID and entity ID). The auto-generated ID and the signature form the full ID. In order to remove an entity, the full ID needs to be supplied.

Previously, when creating a new timeline item, only the auto-generated ID was stored. As a consequence, trying to remove an entity using this ID failed. Now, when creating a new item, we wait for the API response that includes the full entity ID and use that for subsequent requests.
I have kept the old implementation around while developing and have selectively used bits and pieces of the existing implementation. That is why there was a src/components/Timeline directory with the old implementation and a src/components/Timeline2 directory with the new implementation.
@tillprochaska tillprochaska marked this pull request as ready for review January 29, 2023 21:34
@tillprochaska
Copy link
Contributor Author

Not a massive fan of the Timeline2, but that is just me, and by no means a request for change or a need to refactor later.

This is now gone -- I removed the old implementation and renamed Timeline2 to just Timeline. :)

@Rosencrantz Rosencrantz merged commit 12d9d6a into develop Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants