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

Added Verse Ref View Extension #1

Merged
merged 43 commits into from
Apr 18, 2024
Merged

Added Verse Ref View Extension #1

merged 43 commits into from
Apr 18, 2024

Conversation

tjcouch-sil
Copy link
Member

@tjcouch-sil tjcouch-sil commented Apr 17, 2024

Simple little web view that displays the current verse reference. Mainly to help Mac users test Verse Reference stuff especially later when we get scroll groups going.

image


This change is Reviewable

tjcouch-sil and others added 30 commits December 4, 2023 17:47
…itting into its own repo as this problem will not be solved as long as the package used is local
- also add Volta node version
- also update other npm packages
Copy link
Contributor

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

This was quick - must have had it already in the wings.

Reviewed 36 of 36 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @tjcouch-sil)


src/verse-ref-view/.eslintrc.js line 10 at r1 (raw file):

    // Make sure this is last so it gets the chance to override other configs.
    // See https://github.com/prettier/eslint-config-prettier and https://github.com/prettier/eslint-plugin-prettier
    'plugin:prettier/recommended',

BTW: Strictly you don't need this as it's included in erb. You only need it if erb isn't the last item extended (it's currently first and last).


src/verse-ref-view/contributions/menus.json line 11 at r1 (raw file):

        "group": "platform.projectResources",
        "order": -85,
        "command": "verseRefView.open"

This menu item is not visible for me so I couldn't see all your good work.


src/verse-ref-view/src/verse-ref.web-view.tsx line 38 at r1 (raw file):

const defaultScrRef: ScriptureReference = { bookNum: 1, chapterNum: 1, verseNum: 1 };

global.webViewComponent = function VerseImageGenerator({

This function seems incorrectly named. Copy and paste error?

Code quote:

VerseImageGenerator

src/verse-ref-view/src/verse-ref.web-view.tsx line 78 at r1 (raw file):

  // Get the current verse from the project
  const [verse] = useProjectData('ParatextStandard', projectId).VerseUSFM(verseRef, '');

BTW: I'm guessing this won't currently work on macOS but behaves nicely when it doesn't.

Copy link
Member Author

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 35 of 36 files reviewed, 2 unresolved discussions (waiting on @irahopkinson)


src/verse-ref-view/.eslintrc.js line 10 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW: Strictly you don't need this as it's included in erb. You only need it if erb isn't the last item extended (it's currently first and last).

Ah interesting, this is here on the paranext-core one to overwrite storybook's rules paranext/paranext-core@d72f363 I didn't realize that when making this template eslintrc. I'd say let's just leave it at this point. Or if someone wants to make a template change, that's totally fine.


src/verse-ref-view/contributions/menus.json line 11 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This menu item is not visible for me so I couldn't see all your good work.

Did you have fix-web-view-menus checked out on paranext-core? None of this works without it because of the bug I fixed in that (and the localized string added).


src/verse-ref-view/src/verse-ref.web-view.tsx line 38 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This function seems incorrectly named. Copy and paste error?

Good call. Fixed


src/verse-ref-view/src/verse-ref.web-view.tsx line 78 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW: I'm guessing this won't currently work on macOS but behaves nicely when it doesn't.

Right haha it probably doesn't show anything on mac. I just figured might as well throw it in so windows (and hopefully one day mac) users see something interesting in the web view.

Copy link
Contributor

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

:lgtm:

BTW: if it matters the package-lock.json file isn't up-to-date.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @tjcouch-sil)


src/verse-ref-view/contributions/menus.json line 11 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Did you have fix-web-view-menus checked out on paranext-core? None of this works without it because of the bug I fixed in that (and the localized string added).

Yes, I also tried that yesterday figuring it was related. Haha, I think I forgot to checkout this branch yesterday since I didn't notice that the package-lock.json file wasn't up-to-date yesterday. It's working now 😁 .

@tjcouch-sil tjcouch-sil merged commit a8616ba into main Apr 18, 2024
1 check was pending
@tjcouch-sil tjcouch-sil deleted the scr-ref-extension branch April 18, 2024 21:46
Copy link
Member Author

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Oops. I think maybe we can just wait til next PR if that's ok. Thanks for the review!

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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.

4 participants