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 all JS to Stimulus #634

Closed
4 tasks done
blms opened this issue Feb 21, 2022 · 9 comments
Closed
4 tasks done

Refactor all JS to Stimulus #634

blms opened this issue Feb 21, 2022 · 9 comments
Assignees
Labels
🛠️ chore One-off task or update
Milestone

Comments

@blms
Copy link
Contributor

blms commented Feb 21, 2022

dev notes

Stimulus was first used for the relevance sort in #550, the following should also be refactored to use Stimulus:

@blms blms added the 🛠️ chore One-off task or update label Feb 21, 2022
@blms blms changed the title Refactor all JS to Stimulus / Turbo Refactor all JS to Stimulus Feb 21, 2022
@rlskoeser
Copy link
Contributor

@blms regarding your note about search.js "extending" the search controller — we should just port the existing logic over to the search controller, right?

@blms
Copy link
Contributor Author

blms commented Feb 21, 2022

@rlskoeser exactly!

@rlskoeser
Copy link
Contributor

@blms what about openseadragon init?

@blms
Copy link
Contributor Author

blms commented Feb 21, 2022

@rlskoeser Ah, I didn't think to include it since it's just injecting OSD vendor code into the HTML, but it probably should be included, I'll add it to the list!

@rlskoeser
Copy link
Contributor

We may want to split this out into separate subtasks, but can wait and see how it goes; am wondering if the seadragon should be more of a progressive enhancement. I was looking at the site on my phone and wasn't seeing images on detail pages — which we should check on — but made me wonder if you really want deep zoom by default on mobile.

@blms
Copy link
Contributor Author

blms commented Feb 21, 2022

@rlskoeser Good call, and I see the mobile issue—it occurs on pages where there are images but no transcription. I'll track it as a bug!

@rlskoeser
Copy link
Contributor

@blms great — thanks for documenting, we should get that resolved

blms added a commit that referenced this issue Mar 1, 2022
blms added a commit that referenced this issue Mar 2, 2022
blms added a commit that referenced this issue Mar 7, 2022
@blms blms self-assigned this Mar 7, 2022
blms added a commit that referenced this issue Mar 9, 2022
blms added a commit that referenced this issue Mar 9, 2022
rlskoeser added a commit that referenced this issue Mar 11, 2022
@rlskoeser
Copy link
Contributor

confirmed in qa, but noticed we're still including a main.js bundle that appears to be empty — let's remove that one before we close this

@rlskoeser rlskoeser added this to the v4.3 milestone Mar 11, 2022
@rlskoeser
Copy link
Contributor

confirmed main.js removed in qa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛠️ chore One-off task or update
Projects
None yet
Development

No branches or pull requests

2 participants