-
Notifications
You must be signed in to change notification settings - Fork 3
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
put display mode querystring #40
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paltman This works great for display modes, but the "subdisplay" settings are not persisted in the URL.
"subdisplay" as referenced in scaife-viewer/frontend/#25 (and discussed in this comment on #26 pertains to any toolbar or selection options within a display mode.
So if I'm viewing a passage in "map mode" (/explore-homer/urn:cts:greekLit:tlg0012.tlg001.perseus-eng4:2.480?mode=named-entities
), we should persist that I've chosen to view the map in parallel:
or in "alignments" mode, that I've selected the sentence level alignment:
/explore-homer/urn:cts:greekLit:tlg0012.tlg001.perseus-grc2:1.1-1.7?mode=alignments
Other examples I can think of off the top of my head (but double check each display mode component):
- "Folio Images" also has subdisplay state (for which columns are shown)
- "Syntax trees" (what is expanded / collapsed)
(And I'm open to introducing a better name than subdisplay if you have any ideas...maybe just calling it display mode state is clearer)
@jacobwegner should be ready to go now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
As noted in my comments above; there may be some patterns we want to establish in the future as part of shoring up the widget "contract" (what querystring params the widget expects / how it cleans up when destroyed, cataloging options in a constants module, etc).
@jacobwegner I pushed something that encapsulates the linking so we gain:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paltman love the encapsulation you added!
I think it also sets us up nicely if we need to further site-level overrides down the line (if we need to expose additional config, for example).
No description provided.