feat: allow blocks lower than units to be bookmarked #35207
Conversation
|
Thanks for the pull request, @navinkarkera! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
the
open-source-contribution
mphilbrick211
added
the
needs reviewer assigned
navinkarkera
force-pushed
the
navin/bookmark
branch
from
3c11544
to
80a3629
Compare
| ).always(function() { | ||
| window.location.href = event.currentTarget.pathname; | ||
| }); | ||
| ); | ||
| }, |
There was a problem hiding this comment.
@navinkarkera I am not able to understand this, how is the navigation happening now?
There was a problem hiding this comment.
Now I think I understand this function is called when we click a bookmark, it's just a way to log it, but it just makes me wonder do we had to do a window.location.href I am just trying to figure the reasoning out of it.
There was a problem hiding this comment.
@farhaanbukhsh My best guess is that it was done to log the event first and then navigate but this way we loose query params. It felt weird to navigate using javascript when it is already handled by the browser link element.
There was a problem hiding this comment.
I understand my only concern was are we missing some cases, but it doesn't seem like it.
navinkarkera
force-pushed
the
navin/bookmark
branch
from
80a3629
to
67c46c9
Compare
navinkarkera
force-pushed
the
navin/bookmark
branch
from
67c46c9
to
7bf1df7
Compare
|
Hi @navinkarkera! Just flagging that a few additional tests have popped up. |
Adds support to jump/scroll to blocks lower than units if bookmarked. It also fixes an issue with bookmark visit url function where it was not passing query params.
navinkarkera
force-pushed
the
navin/bookmark
branch
from
7bf1df7
to
d0361e8
Compare
Description
Adds support to jump/scroll to blocks lower than units if bookmarked. It also fixes an issue with bookmark visit url function where it was not passing query params.
Supporting information
Private-ref: BB-8950Testing instructions
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.