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

Refactor Get Resources into Filterable Resource List PBR component #1363

Merged
merged 21 commits into from
Dec 11, 2024

Conversation

rolfheij-sil
Copy link
Contributor

@rolfheij-sil rolfheij-sil commented Dec 2, 2024

This PR:

  • moves the Get Resources extension from Platform Bible Internal Extensions into Core
  • creates a new Home extension that is basically a copy of Get Resources for now
  • refactors Get Resources into a reusable component named Filterable Resource List

This PR does not update the Get Resources implementation in any way, so the code in FilterableResourceList.component.tsx has already been reviewed in this PR

This change is Reviewable

… 93d4b0a63

git-subtree-dir: extensions/src/platform-get-resources
git-subtree-split: 93d4b0a6364a69340364e47ddfb3c58e183e49c5
git-subtree-dir: extensions/src/platform-home
git-subtree-split: 93d4b0a6364a69340364e47ddfb3c58e183e49c5
@rolfheij-sil rolfheij-sil changed the title Get Resources (moved) and Home extensions Refactor Get Resources into Filterable Resource List PBR componert Dec 6, 2024
@rolfheij-sil rolfheij-sil changed the title Refactor Get Resources into Filterable Resource List PBR componert Refactor Get Resources into Filterable Resource List PBR component Dec 6, 2024
@rolfheij-sil rolfheij-sil marked this pull request as ready for review December 6, 2024 16:21
Copy link
Member

@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.

Looking good! Thanks :) just a few little things.

Reviewed 75 of 100 files at r1, 29 of 29 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @rolfheij-sil)


extensions/src/platform-get-resources/LICENSE line 3 at r2 (raw file):

MIT License

Copyright (c) <year> <copyright_holder_name>

Need to fill license copyrights in :)


extensions/src/platform-home/LICENSE line 3 at r2 (raw file):

MIT License

Copyright (c) <year> <copyright_holder_name>

Copyright


assets/localization/en.json line 7 at r2 (raw file):

  "%about_db_ip_attribution_intro%": "Internet safety features use data from",
  "%about_db_ip_attribution_terms%": "Terms",
  "%downloadResources_errorRegistrationInvalid%": "User registration is not valid. Cannot retrieve resources from DBL.",

Technically we're not supposed to remove localized strings because that's a breaking change 😬 it's fine this once since these are suuuuper specific strings that probably haven't been and won't be used elsewhere. But let's avoid removing strings in the future :)


extensions/src/platform-get-resources/src/main.ts line 23 at r2 (raw file):

    return {
      allowPopups: true,

What is this for? I think it may not be needed


extensions/src/platform-home/README.md line 5 at r2 (raw file):

Home screen extension for Platform.Bible

<!-- <!-- Opening comment tag for Template Info Section. Ignore this for now. More info in [Hide Template Info](#hide-template-info). -->

Commented?


extensions/src/platform-home/package.json line 94 at r2 (raw file):

  },
  "volta": {
    "node": "20.18.0"

Volta extend


extensions/src/platform-get-resources/README.md line 5 at r2 (raw file):

Get resources extension for Platform.Bible

<!-- <!-- Opening comment tag for Template Info Section. Ignore this for now. More info in [Hide Template Info](#hide-template-info). -->

I think this section still need to be hidden


extensions/src/platform-home/src/main.ts line 23 at r2 (raw file):

    return {
      allowPopups: true,

Also theorize this may not be needed


extensions/src/platform-get-resources/package.json line 93 at r2 (raw file):

    "zip-build": "^1.8.0"
  },
  "volta": {

To match other extensions, please change this to the following:

"volta": {
    "extends": "../../package.json"
  }

Copy link
Contributor Author

@rolfheij-sil rolfheij-sil left a comment

Choose a reason for hiding this comment

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

Thanks for reviewing! :)

Reviewed 74 of 100 files at r1, 12 of 29 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @tjcouch-sil)


assets/localization/en.json line 7 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Technically we're not supposed to remove localized strings because that's a breaking change 😬 it's fine this once since these are suuuuper specific strings that probably haven't been and won't be used elsewhere. But let's avoid removing strings in the future :)

Oops. Noted.


extensions/src/platform-get-resources/LICENSE line 3 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Need to fill license copyrights in :)

Done.


extensions/src/platform-get-resources/package.json line 93 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

To match other extensions, please change this to the following:

"volta": {
    "extends": "../../package.json"
  }

Done.


extensions/src/platform-get-resources/README.md line 5 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I think this section still need to be hidden

Done


extensions/src/platform-get-resources/src/main.ts line 23 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

What is this for? I think it may not be needed

Don't know where that came from. Maybe I copied it from something else in Platform Bible Internal Extensions. Removed it now.


extensions/src/platform-home/LICENSE line 3 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Copyright

Done.


extensions/src/platform-home/package.json line 94 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Volta extend

Done.


extensions/src/platform-home/README.md line 5 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Commented?

Done


extensions/src/platform-home/src/main.ts line 23 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Also theorize this may not be needed

Done.

Copy link
Contributor Author

@rolfheij-sil rolfheij-sil left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 100 files at r1, 17 of 29 files at r2.
Reviewable status: 96 of 104 files reviewed, 6 unresolved discussions (waiting on @tjcouch-sil)

Copy link
Contributor Author

@rolfheij-sil rolfheij-sil left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r3, all commit messages.
Reviewable status: 96 of 104 files reviewed, 6 unresolved discussions (waiting on @tjcouch-sil)

tjcouch-sil
tjcouch-sil previously approved these changes Dec 10, 2024
Copy link
Member

@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.

:lgtm: woo! I didn't follow the discussion super carefully; did you ever get an answer on what to do with the button to open the download resources page if you're in Platform.Bible?

Reviewed 8 of 8 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

# Conflicts:
#	lib/platform-bible-react/dist/index.cjs
#	lib/platform-bible-react/dist/index.cjs.map
#	lib/platform-bible-react/dist/index.js
#	lib/platform-bible-react/dist/index.js.map
Copy link
Contributor Author

@rolfheij-sil rolfheij-sil left a comment

Choose a reason for hiding this comment

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

Yay, thanks! Nope, it's gonna be part of a larger discussion. UX indicated that they currently do not have clear distinctions between designs for Platform and designs for Paratext. They will have to think this through and come to some decisions. I don't know when that will happen

Reviewable status: 96 of 104 files reviewed, all discussions resolved (waiting on @tjcouch-sil)

Copy link
Contributor Author

@rolfheij-sil rolfheij-sil left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r4, all commit messages.
Reviewable status: 96 of 104 files reviewed, all discussions resolved

Copy link
Member

@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.

Reviewed 8 of 8 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

tjcouch-sil
tjcouch-sil previously approved these changes Dec 11, 2024
Copy link
Member

@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: :shipit: complete! all files reviewed, all discussions resolved

# Conflicts:
#	lib/platform-bible-react/dist/index.cjs
#	lib/platform-bible-react/dist/index.cjs.map
#	lib/platform-bible-react/dist/index.js
#	lib/platform-bible-react/dist/index.js.map
Copy link
Member

@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.

:lgtm:

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@rolfheij-sil rolfheij-sil merged commit 86e485a into main Dec 11, 2024
7 checks passed
@rolfheij-sil rolfheij-sil deleted the 1290-v2-resources-and-home-extension branch December 11, 2024 19:09
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