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

[reader] Refactor reader.js #410

Merged
merged 13 commits into from
Oct 8, 2022
Merged

[reader] Refactor reader.js #410

merged 13 commits into from
Oct 8, 2022

Conversation

akprasad
Copy link
Contributor

@akprasad akprasad commented Oct 7, 2022

This commit refactors our reader code to better make use of Alpine. The changes here impose a simple reactive dataflow where the source of truth is whatever is stored on the Alpine application object.

Why do this now?

The reader.js code is manageable but just barely so. The current data model makes it difficult to reason about the application state and write tests. By doing more of the heavy lifting in Alpine and its reactive data model, we can simplify several parts of the code while maintaining the same functionality and making it easier to build new features.

Summary of changes

HTML:

  • Use x-cloak, template, and other Alpine devices to more compactly describe JS-specific logic.
  • Include serialized JSON data in texts/section.html, which we use to initiaiize the Alpine app.
  • Move various HTML fragments from reader.js into texts/section.html.

JavaScript:

  • Move all application data int the Alpine application. This is all we need to do for Alpine to add reactive data bindings for our application.
  • Transliterate by calling transliterateHTML and transliterateStr functions that depend on this.script. Application data is always stored in Devanagari, which means that we can now support scripts like Gurmukhi and Bengali that don't losslessly round-trip. Because of the dependency on this.script, these functions will rerun whenever this.script changes.
  • Remove this.uiScript, since it is no longer necessary.
  • Move common string constants into the pseudo-enums Layout and Script.

Python:

  • Add support for serializing dataclasses by simply using .asdict().
  • [unused] Add a new API endpoint that fetches JSON data for a section. I used this during testing, but it's no longer on any production path. However, we'll pick it up in a future PR.

CSS:

  • Remove the previous side-by-side and in-place classes and move this logic into reader.js

Overall:

  • Add various comments to make the logic here clearer.

Differences from prod

  • [feature] Add support for Bengali and Gurmukhi.
  • [regression] Text and section titles are not transliterated. I'll fix this in a follow-up PR to avoid adding to an already complex PR.
  • [css] Minor changes to the side-by-side layout.
  • [css] Minor color and spacing changes in the dictionary sidebar.

Future plans

  • Support transliteration for text and section titles.
  • Add transitions from section to section for a nicer UX. (The idea is that after initial load, each request is just for JSON data as opposed to the entire page.)
  • Encode application state in the URL so that we can link to specific views (e.g. sidebar open, specific word clicked)
  • When a verse is clicked, show padaccheda only for the clicked chunk of text, as opposed to the entire block. This could potentially let us remove the in-place vs. side-by-side logic if the UX satisfies.

Test plan

I added unit tests to increase the overall statement coverage for reader.js from 30% to 50%. In addition, I tested the following behaviors on dev:

  • Simple page load.
  • Switch between multiple scripts.
  • Switch between multiple parse layouts.
  • Click on multiple block.
  • Click on multiple parsed words.
  • Various permutations of the above.

@akprasad akprasad changed the title [reader] Reafctor reader.js [reader] Refactor reader.js Oct 7, 2022
@akprasad
Copy link
Contributor Author

akprasad commented Oct 7, 2022

(This fixes #12 and #42.)

@akprasad
Copy link
Contributor Author

akprasad commented Oct 8, 2022

24h, merging

@akprasad akprasad merged commit b92cf2f into main Oct 8, 2022
@akprasad akprasad deleted the reader--spa branch October 8, 2022 02:24
Copy link
Contributor

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! sorry I got to this late



@api.route("/texts/<text_slug>/<section_slug>")
def reader_json(text_slug, section_slug):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test in test_texts.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, added in #415.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants