-
Notifications
You must be signed in to change notification settings - Fork 270
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tillprochaska
force-pushed
the
till/timelines
branch
6 times, most recently
from
October 24, 2022 16:26
eb7b7b5
to
8782f45
Compare
tillprochaska
force-pushed
the
till/timelines
branch
from
October 31, 2022 17:24
14259d8
to
d3b7dd9
Compare
tillprochaska
force-pushed
the
till/timelines
branch
4 times, most recently
from
November 11, 2022 10:22
51ed7bf
to
f75a753
Compare
tillprochaska
force-pushed
the
till/timelines
branch
4 times, most recently
from
December 27, 2022 16:15
2a24a87
to
caf8130
Compare
tillprochaska
commented
Dec 27, 2022
tillprochaska
force-pushed
the
till/timelines
branch
5 times, most recently
from
December 29, 2022 20:04
a15fa05
to
e1905ca
Compare
tillprochaska
force-pushed
the
till/timelines
branch
3 times, most recently
from
January 6, 2023 16:04
d5f9b50
to
d8af93a
Compare
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
force-pushed
the
till/timelines
branch
from
January 29, 2023 21:32
f90a923
to
3a45add
Compare
This is now gone -- I removed the old implementation and renamed |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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 arenderer="chart"
property here.How to review
I’d recommend reviewing in the following order:
src/components/Timeline2/types.ts
and the classes (TimelineItem
,ImpreciseDate
) insrc/components/Timeline2/util.ts
.src/screens/TimelineScreen/TimelineScreen.jsx
src/components/Timeline2/Timeline.tsx
andsrc/components/Timeline2/Timeline.test.tsx
Timeline
component as well as respective tests.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 asrc/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:
src/react-ftm/components/NetworkDiagram/toolbox/EntityViewer.tsx
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:
Rebase and reenable type checking in tests where we use window.cryptoBefore merging:
Timeline
directory and renameTimeline2
directoryPotential enhancements:
TimelineItem
inEntityViewer
Screenshots