-
Notifications
You must be signed in to change notification settings - Fork 172
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
Update links from Delegate Crash Course to Delegate Handbook #9308
Update links from Delegate Crash Course to Delegate Handbook #9308
Conversation
Hi @nsilvestri, may I know by when are you planning to get this PR merged? Does WQAC has any deadline for the same? The reason why I'm asking is, in /panel, Delegate Panel is going to be migrated to a separate react component. I've already raised the PR to migrate Delegate Panel: #9332. Once that is merged, you should be able to update delegate-crash-course to delegate-handbook more easily probably. |
@danieljames-dj this PR will be pending the Delegate Handbook being added to wca-documents, hopefully 2ish weeks? Don't let this PR be a blocker for making any changes to the panel, I can adapt to whatever changes happen in the meantime. |
5ab002c
to
2359351
Compare
d9fc13b
to
eb03891
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me. I've raised a comment at the place where I have a slight concern but not sure if it's solvable. I would like @gregorbg to have a look to give the final approval.
Thanks @nsilvestri for working on this. 🎉
<%= link_to "'Submitting competition results' section of the delegate crash course", | ||
panel_delegate_crash_course_path %>. | ||
<%= link_to "'Competition Results' section of the Delegate Handbook", | ||
"https://documents.worldcubeassociation.org/edudoc/delegate-handbook/delegate-handbook.pdf#competition-results" %>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I tried opening this link, but it's not scrolling to the section in the handbook. Is this something that is supported in at least some browsers?
- Having the same URL hardcoded in two places looks bit odd, but I cannot think of a way to solve it especially because there is a hash parameter. Just mentioning here so that you or somebody else will be able to find a solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Yes; on Firefox the link takes me straight to the section. It should be supported in other browses as AFAIK it's the same as heading tag hashes in URLs, but if it doesn't work in some browser configuration that's nbd to me since otherwise the link works and the section is explicitly called out here.
- I'm not sure how you'd prefer to handle a constant like this across Rails code and React code, open to suggestions if it's a priority fix. Changing this around would also be easy in a subsequent commit if required.
I'm approving this for now, if any solution for the URL comes up, we can take it up later. |
The Delegate Crash Course has been deprecated in favor of the Delegate Handbook. This PR updates existing references to the DCC to the Handbook where it is still relevant, or removes references to the DCC entirely where they are no longer needed.