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

Removed inline js from breadcrumb_select.html. #8447

Merged
merged 5 commits into from
Oct 24, 2023
Merged

Removed inline js from breadcrumb_select.html. #8447

merged 5 commits into from
Oct 24, 2023

Conversation

mudit-loya
Copy link
Contributor

@mudit-loya mudit-loya commented Oct 20, 2023

Added and improved functionality of inline javascript in breadcrumb_select.js file. Imported this breadcrumb_select.js file in index.js.

Closes #8380 Remove inline js: books/breadcrumb_select.html

Refactor

Technical

Removed inline js from breadcrumb_select.html.
Updated index.js and created a file openlibrary/plugins/openlibrary/js/breadcrumb_select/index.js for using the inline js functionality.

Testing

  1. Visit http://localhost/account/books/currently-reading
  2. In the select options available try pressing the keys 'ArrowUp', 'ArrowDown' to prevent changes.
  3. In the select options available try pressing the keys 'Tab', 'Enter', ' ' to allow changes to take place.

Screenshot

Screenshot from 2023-10-24 18-37-31
Screencast from 2023-10-24 18-37-44.webm

Stakeholders

@RayBB

Added and improved functionality of inline javascript in breadcrumb_select.js file.
Imported this breadcrumb_select.js file in index.js.
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2023

Codecov Report

Merging #8447 (163df60) into master (fbdb3a5) will decrease coverage by 0.07%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #8447      +/-   ##
==========================================
- Coverage   15.90%   15.83%   -0.07%     
==========================================
  Files          85       86       +1     
  Lines        4635     4653      +18     
  Branches      814      818       +4     
==========================================
  Hits          737      737              
- Misses       3381     3395      +14     
- Partials      517      521       +4     
Files Coverage Δ
openlibrary/plugins/openlibrary/js/index.js 0.00% <0.00%> (ø)
.../plugins/openlibrary/js/breadcrumb_select/index.js 0.00% <0.00%> (ø)

@mudit-loya
Copy link
Contributor Author

@RayBB any changes?

@RayBB
Copy link
Collaborator

RayBB commented Oct 20, 2023

@imperial-chief can you please provide instructions for testing and screenshots of it working?

@RayBB
Copy link
Collaborator

RayBB commented Oct 20, 2023

The testing should be go to a page like "currently reading" and then use dropdown to move to another page.
Also, I think we should add something like this but with document.querySelectorAll('.crumb select');

Overall it looks good though!

@mudit-loya
Copy link
Contributor Author

Okay i will try doing so.

@imperial-chief can you please provide instructions for testing and screenshots of it working?
What do you mean by 'please provide instructions for testing'

@RayBB
Copy link
Collaborator

RayBB commented Oct 21, 2023

Someone else who isn't me will test the PR. We should write steps for them to test it so they don't need to figure out what page the code runs on.

@mudit-loya
Copy link
Contributor Author

@RayBB i tried finding the usage of this,
But i could find exactly where this breadcrumbs are used.
Can you help me find it?

@mudit-loya
Copy link
Contributor Author

@RayBB let me know for any changes further.

@RayBB
Copy link
Collaborator

RayBB commented Oct 23, 2023

@imperial-chief the code looks good.
Can you please test that it works and provide screenshots and testing instructions.
The breakcrumbs are found on a page like https://openlibrary.org/account/books/currently-reading
In this dropdown:
image

@mudit-loya
Copy link
Contributor Author

@RayBB i tried running the changes but i failed.
Not only that, while debugging i tried to find where was my code breaking and found that index.js wasn't getting executed at all (i added many console.log() statements in complete index.js but i wasn't able to see any of them).
Can you please help me out with this?

@RayBB
Copy link
Collaborator

RayBB commented Oct 24, 2023

@imperial-chief did you try rebuilding the JS?
This can be done by running: docker compose run --rm home make js

Or docker-compose run --rm home npm run-script watch which will watch for changes to the JS file and recompile after each change.

A bit more info on docker here: https://github.com/internetarchive/openlibrary/blob/master/docker/README.md

If you're still having trouble you can watch some of the onboarding videos at https://github.com/internetarchive/openlibrary/blob/master/CONTRIBUTING.md

@mudit-loya
Copy link
Contributor Author

@RayBB i uploaded a screenshot and a screencast link of testing.
I also updated the testing instructions in the PR main comment.

@RayBB
Copy link
Collaborator

RayBB commented Oct 24, 2023

Looks good to me. Now we just need @jimchamp to review and merge :)

@mudit-loya
Copy link
Contributor Author

Sure!

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 @imperial-chief! Will merge after I commit my suggested changes.

openlibrary/templates/books/breadcrumb_select.html Outdated Show resolved Hide resolved
@jimchamp jimchamp merged commit 08c5f93 into internetarchive:master Oct 24, 2023
4 checks passed
@mudit-loya mudit-loya deleted the 8380/refactor/remove-inline-js-breadcrumb branch October 28, 2023 09:05
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.

Remove inline js: books/breadcrumb_select.html
4 participants