Show comparisons between versions in history viewer #33
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
In order to compare two form fields, there needs to be a way to load data in such a way that it will allow comparison between two values. To achieve this a form transformation has been added to faciliatate both loading two sets of data values, and rendering a differential of those values in a read only field.
In order to inform a user that they are in compare mode, and are either viewing the difference between two versions, or are required to select two version to compare. The notice includes a handy exit button to leave compare mode.
In order to allow a user to enter or exit compare mode there is a button on the heading row that will reveal more actions when clicked. At current the only action present is a toggle to enter or exit compare mode, and reflect the state (check means compare mode is active).
History viewer is the parent of all fields related to comparison mode viewing, and begins the process by deciding whether to render a detail view or a list of versions. It now must also factor comparison mode into this, and so alter the way it handles the list or the detail view.
And add to detail view as well as the list view.
Enable compare mode handling for list view/selection, and detail views, then connect the PHP diff form to history view comparison so the components can actually serve real data. This results in being able to open a comparison.
… and form scaffolding methods
NEW Add pointer cursor for versions in the list on hover FIX Ensure toolbar--condensed class is correctly removed when component unmounts FIX Correct eslint violations and provide component in broken unit test
Updating implementation to not adjust the reducer, and preserve the state before you entered compare mode. Fixing up CSS specificity & removing !important. Fix the UI when choosing to select two versions to compare from the list screen. I also made the actions column always render, because is was causing some awkward jittering as things became visible.
Major refactor of Diff Form Transformation The original approach was implmented as a "get it functional" state, and a bit of an attempt to circumvent the complexities involved with the logical order of operations, the nature of a failover (see ViewableData), and having form fields work in more than one display facet. However there were limitations in handling more complex data structures such as when a value from the database was NULL, this would fail a data comparison check between loaded data and transformation data. In order to work around this we now simply ignore all these complexities and pass off to normal errors or exceptions that will be output with such use. There is now a diff field that will handle the data explicitly, rather than having the entire transformation happen in the transformer class. This was originally avoided due to a failover being an underlay, not an overlay, so only methods that didn't exist on the superclass would be passed off to the form field we're tryign to transform, instead of for any method not defined on the class. To circumvent this issue, we now simply don't (cicumvent this issue), and rely on users not e.g. transforming a form then attempting to update one of the old fields.
…acted to a component
As well, component class names are updated to use BEM conventions and reduce CSS nesting levels
3 tasks
maxime-rainville
requested changes
Aug 7, 2018
We've been subtasking this A/C out of each ticket so far into #1 |
|
I've added aria roles to the list and list items, as well as the columns and cells within it, and ensured that it's keyboard accessible (#20). The Behat tests are failing because the GraphQL queries are no longer running since GraphQL v3 was introduced. I'll need to update this again I think. |
|
Ok @maxime-rainville I've investigated this.
Recapping that permalinked URL A/Cs (from all history viewer epics/stories) so far have been removed and subtasked to #1 for future consideration. Note that there are 5 PRs unmerged for this issue, testing should include them all |
|
Wait, I've done bad things with the keyboard accessibility... please hold caller |
|
I've updated to switch using an anchor for a span inside list items so that (A) it isn't an anchor with no |
robbieaverill
force-pushed
the
pulls/1/different-rebase
branch
from
7212bc7 to
eed1ca9
Compare
This was referenced Aug 10, 2018
maxime-rainville
approved these changes
Aug 12, 2018
|
Thanks for the review @maxime-rainville! I'd like to get Travis green before we merge |
…te column is unaffected
robbieaverill
force-pushed
the
pulls/1/different-rebase
branch
from
0d3208b to
8611e47
Compare
|
@raissanorth FYI I've run rebuilt the dist files from your last commit and squashed it, new hash 8611e47 |
…hor element container
|
I've updated the behat tests. There's one scenario which is still correctly failing since required dependency silverstripe/silverstripe-cms#2221 hasn't been merged yet. |
|
Builds restarted, should be green now |
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.
Adds the ability to enter "compare mode" when viewing the history for a particular record, allowing a user to select two versions and be presented with a visual comparison between them, according to value by default.
For example
"The Title"becoming"Title Story" would show "The" struck out on a red background (indicating removal), "Title" as normal text, and "Story" on a green background (indicating insertion).These comparisons are always from the older version to the newer one, chronologically.
Resolves silverstripe/silverstripe-versioned#96, resolves #30, resolves #20 and resolves #17