next release #1193
Merged
next release #1193
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
using redux to cache JSONs
this completes a bunch of TODOs (search TODO:1071 for remaining items): 1. clear the json cache when unmounting narrative component 2. rerender mounted react components with new dataset upon redux state update this is achieved using react "key" props (https://reactjs.org/docs/lists-and-keys.html#keys) 3. wait for all datasets to be fetched using promise.all 4. fix loading narratives from not the first page 5. use the appropriate dataset when leaving the narrative page to explore a dataset
also rework narrative json fetching code, fixes bug with: "explore the data yourself" and "return to narrative" buttons
… file This is in preparation for client-side parsing of narrative files. A optional `type` URL query allows the same API to return the markdown file (see the documentation changes in this commit for more details). There are other possibilities, such as using the content-type field in the header, but I felt this was the clearest.
Here we add the framework to (eventually) parse the response from `getNarrative` as a markdown file and interpret this client-side. To facilitate backward compatibility in the cases where a server does not respect the `?type=md` query and continues to return a JSON, if the file doesn't look like markdown we will attempt to parse it as JSON. To mimic such a server, we explicitly request "type=json" in this commit, as the code to actually do the client-side md-parsing is not yet implemented.
This commit shifts the parsing of the markdown file (md + yaml frontmatter) into a centralised function which is imported by both the server and the client, allowing us to maintain backward compatible server-side parsing as well as client-side parsing, whilst avoiding code duplication and the inevitable divergence of functionality. Our approach to conversion of YAML frontmatter -> markdown is essentially unchanged. The algorithm for parsing of the main body has been rewritten to be more robust. Unit tests are added to cover the various ways we can define authors, links & affiliations in the YAML frontmatter of narratives.
Parsing of the narrative (YAML) frontmatter produces markdown with `<sup>` and `<sub>` HTML elements to render affiliations using superscripts. (This behavior is unchanged from parsing server-side.) The client-side markdown sanitizer settings, however, were stricter than those employed on the server, and were removing these HTML elements causing the affiliations not to be displayed. These settings are relaxed in this commit.
The move to client-parsing of narratives has caused two changes to bundle structure. Firstly, since the `loadJSON` function now references the markdown parsing code, the dependency graph tries to pull in the related libraries into the chunk with most of the Auspice code (currently chunk 5). Previously these were a separate chunk lazily loaded with (e.g.) the footer parsing component. Since it's very common to display markdown footers, it makes life simple to include these libraries in the other-vendors bundle. It is possible to lazily load the markdown parsing function as needed, but more complicated. Secondly, the YAML frontmatter parser (previously server-side only) has a dependency on `js-yaml` which itself has dependency on "esprima" and others. Esprima is unnecessary for our purposes and we use a webpack loader to ignore it. For simplicity, we shift `js-yaml` to the other-vendors bundle. An alternative approach would be to bundle pre-minified 'js-yaml' bundle.
For unknown reasons, the core-vendors and other-vendors chunks have different hashes when building locally compared with via GitHub CI. This is after running `npm ci` to mimic the CI conditions. An issue should be created to better understand this, as it implies that the contents differ.
* parse-narrative-in-client: [client-narratives] update bundlesize config [client-narratives] change bundle structure to accommodate YAML parsing [client-narratives] modify markdown parser settings [client narratives] update documentation [client-narratives] parse markdown client-side [client-narratives] Allow client to gracefully fall-back to JSON [client-narratives] Allow narrative API to return unmodified markdown file
* 1050-mult-narratives: (26 commits) narrative fetching error handling Add back in CACHE_JSONS remove unused action CLEAR_JSON_CACHE Only clear jsonCache on CLEAN_START; linting Update loadData.js throw 404 when dataset is not available, compatibility fallback for old narratives async fetching of additional datasets, remove _.uniq fetch unique set of datasets for narratives TODOs fix package lock to same as master mostly clarifying comments WIP: remove listeners in components when unmounted clean up + jsonCache redux sensible default mult-dataset narratives working; changing narrative datasets: how to best rerender? WIP: mult-dataset narratives partly working WIP allow multiple dataset narratives; Update recomputeReduxState.js Update recomputeReduxState.js ...
This commit was prompted by a bug resulting from the URL query representing two different concepts: what is displayed in the URL and the app state that this drives.
Previously we approached this with two arguments to the state-setting-function: `query` and `queryToDisplay`. Here we remove the latter and interpret the former as synonymous with the URL. This results in simplified logic where the logic for interpreting what app state a narrative page desires (e.g. `query={n: 3}` -> `{c: "region", ...}) now resides within the `createStateFromQueryOrJSONs` function.
This commit represents a simplification of the page-change logic when navigating between pages (the `EXPERIMENTAL_showMainDisplayMarkdown` action is unnecessary as the `changePage` action will correctly set the panels). Additionally we rename the react component to remove the "experimental" prefix as this has been in widespread use for a number of months now.
|
@eharkins would you mind looking at the just-added commits, especially
I plan on releasing and pushing this up to dev.nextstrain.org for some final testing over the coming days. Multiple-datasets-in-narratives are really cool and we should write a blog-post / how-to guide for these -- if you're keen, then please jump in, otherwise I'll try to get to it as soon as this is released. |
|
James I don't know the time scale you'd like to work to, but I'd be
happy to help with multi-source narrative. However, running this conference
for today and tomorrow!
…On Thu, 30 Jul 2020 at 07:03, james hadfield ***@***.***> wrote:
@eharkins <https://github.com/eharkins> would you mind looking at the
just-added commits, especially
- ba623e1
<ba623e1>
which fixes a bug in the query (the changes in #1164
<#1164> meant that narrative
queries would only modify the controls state, not other bits of state such
as tree, which was why the tree-zooming wasn't working inside narratives)
- 6df515c
<6df515c>
which extends #1164 <#1164>
to work for mobile displays
I plan on releasing and pushing this up to dev.nextstrain.org for some
final testing over the coming days.
Multiple-datasets-in-narratives are really cool and we should write a
blog-post / how-to guide for these -- if you're keen, then please jump in,
otherwise I'll try to get to it as soon as this is released.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1193 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADNA54TDD7QERSKGCLRMZWLR6D5I7ANCNFSM4PLH277A>
.
--
*~~~~~~~~~~~~~~~~~~~~~~~~~~~*
*I'm based in Europe, and sometimes work at irregular times. If this email
arrives outside of your regular working times, please do *not* feel
obligated to respond immediately!*
|
|
Awesome, thanks @emmahodcroft -- given the scale of the code-changes to narratives I expect this to be up on dev.nextstrain.org for at least a few days of testing before release. A blog / example nCoV narrative would be fantastic, and there's absolutely no reason it has to be concurrent with the auspice release. |
|
@jameshadfield sounds good, I'm planning to devote time tomorrow to take a look at those recent commits and any other remaining to-dos. |
P.S. I didn't test using the nextstrain.org server since I equated this with testing on dev.nextstrain.org but if this isn't the case then I'm happy to do that next week! |
This commit represents a big simplification of how we toggle into and out of narrative mode. This happens in two places: the "exit narrative mode" button on the last narrative slide and the "explore the data" button (desktop only). The desired behavior is that we switch to "interactive mode" (i.e. with the controls sidebar) but with a "return to narrative" button (desktop only). The URL should change from a narrative pathname and n=<slide_number> query to the pathname of the dataset & query describing the view settings.
The code is simplified to use a consistent action for all the above behavior ("TOGGLE_NARRATIVE"). This allows multiple datasets to work appropriately, as well as window resizing triggering a switch to mobile display (or vice versa).
This resolves a long-standing development paper-cut where the narrative URL would disappear when hot-reloading (due to the unmounting behavior).
Co-authored-by: eharkins <eli.harkins@gmail.com>
|
Thanks @eharkins 🙌 I hadn't tested the exit-narrative functionality enough. I've made a couple of further simplifications - see commit message for more details. |
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.
This branch contains a somewhat complicated merge of #1172 and #1164 into master. There are a couple of things which require attention before it actually becomes master (and some more testing too!)
cc @eharkins if you have any time to check these that'd be great!