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

fix TypeChanger bug #8374

Merged
merged 6 commits into from
Oct 13, 2023
Merged

fix TypeChanger bug #8374

merged 6 commits into from
Oct 13, 2023

Conversation

RayBB
Copy link
Collaborator

@RayBB RayBB commented Oct 5, 2023

Contributes to #4474

I was investigating moving the JS to be inline but couldn't figure out how the functionality works.
Turns out it didn't work at all. Perhaps because the code is 12 years old and not used too frequently.
This PR makes the code actually work and moves it away from being inline js.

Technical

thinginput comes from here. It doesn't seem to pass the "onclick" field currently, hence the code wasn't working.

Testing

See video

Screenshot

typechanger.1.mp4

Stakeholders

@jimchamp are you interested in reviewing? Since you've helped with this epic before

@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2023

Codecov Report

Merging #8374 (b43c4c7) into master (35d6b72) will decrease coverage by 0.05%.
Report is 4 commits behind head on master.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #8374      +/-   ##
==========================================
- Coverage   16.66%   16.62%   -0.05%     
==========================================
  Files          83       84       +1     
  Lines        4422     4433      +11     
  Branches      757      758       +1     
==========================================
  Hits          737      737              
- Misses       3202     3212      +10     
- Partials      483      484       +1     
Files Coverage Δ
openlibrary/plugins/openlibrary/js/index.js 0.00% <0.00%> (ø)
openlibrary/plugins/openlibrary/js/type_changer.js 0.00% <0.00%> (ø)

@RayBB RayBB marked this pull request as ready for review October 5, 2023 19:39
@RayBB RayBB requested a review from jimchamp October 5, 2023 19:40
@RayBB RayBB added this to Waiting Review/Merge from Staff in Ray's Project Oct 6, 2023
@jimchamp jimchamp self-assigned this Oct 9, 2023
Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

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

Thanks @RayBB! I think that we do still want this functionality.

Will happily merge this once the suggested changes have been made.

openlibrary/plugins/openlibrary/js/index.js Outdated Show resolved Hide resolved
openlibrary/plugins/openlibrary/js/index.js Outdated Show resolved Hide resolved
openlibrary/plugins/openlibrary/js/type_changer.js Outdated Show resolved Hide resolved
openlibrary/plugins/openlibrary/js/type_changer.js Outdated Show resolved Hide resolved
openlibrary/plugins/openlibrary/js/type_changer.js Outdated Show resolved Hide resolved
@jimchamp jimchamp added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Oct 10, 2023
Co-authored-by: jimchamp <jameschamp@acm.org>
@RayBB RayBB requested a review from jimchamp October 10, 2023 21:00
@RayBB
Copy link
Collaborator Author

RayBB commented Oct 10, 2023

Also made one small CSS Improvement

Before

image

After

image

@RayBB RayBB added Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] and removed Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] labels Oct 10, 2023
Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

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

Thanks @RayBB! This looks good to me.

@jimchamp jimchamp merged commit 337b7b0 into internetarchive:master Oct 13, 2023
4 checks passed
Ray's Project automation moved this from Waiting Review/Merge from Staff to Done Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed]
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants