Repository navigation
Links leading outside Studio need to have a pop out icon #4606
Description
🌱 Are you new to the codebase? Welcome! Please see the contributing guidelines.
Observed behavior
Some links that lead outside Studio don’t have a pop-out icon. One example is the “API documentation” link (leading to https://ricecooker.readthedocs.io/en/latest/index.html) in Settings → Account:
There are more links with this problem.
Expected behavior
Links that lead outside the Studio have a pop-out icon. For example, the aforementioned link needs to appear as:
This needs to be fixed for all links.
User-facing consequences
Visually distinguishing links leading to another website is widely recommended practice to minimize the surprise, and is an expected UX pattern in our products.
Guidance
- Revisit all places where
<KExternalLink>'s are used and ensure that all links that lead outside of Studio haveopenInNewTabset to truthy. Relatedly clean uptarget="_blank"since this is taken care of byopenInNewTab. - Note that not all
<KExternalLink>s are necessarily used for off-site navigation. Some of them are used to navigate between Studio sub-apps. Such links shouldn’t have truthyopenInNewTab(and therefore not open in a new tab and neither have a pop-out icon) since from the user’s point of view, they behave like internal links.
Activity
shivam-daksh commentedon Jul 16, 2024
Hey @MisRob , I would like to work on this.
AlexVelezLl commentedon Jul 16, 2024
Hi @shivam-daksh! Thank you for your interest in contributing to solving this Issue! I will assign it to you 👐. Let us know if you have any questions :).
shivam-daksh commentedon Jul 18, 2024
Thanks @AlexVelezLl for assigning me this task. I've started working. I'm new here, so need little more time to explore.
AlexVelezLl commentedon Jul 18, 2024
Sure! @shivam-daksh No rush 😃.
shivam-daksh commentedon Jul 24, 2024
Hey @AlexVelezLl , I'm having trouble in setting up the project despite going through the documentation and following the given instructions. Could you please guide me through?
MisRob commentedon Jul 25, 2024
Hi @shivam-daksh, I'm sorry to hear that. We will try to help. Would you upload terminal output from your setup (ideally full one) and also mentioned what operating system you use?
shivam-daksh commentedon Jul 25, 2024
Hi @MisRob, I've attached the terminal output file along with the screen recording. Please let me know the steps to fix it.
terminal_output.txt
Screen.Recording.2024-07-25.at.1.49.44.PM.mov
AlexVelezLl commentedon Jul 25, 2024
Hey @shivam-daksh, it seems like you are following the wrong docs 😅, can you try following the instructions at https://github.com/learningequality/studio/blob/unstable/docs/local_dev_docker.md? which are instructions for local development. Let us know if you are still experiencing those problems :)
shivam-daksh commentedon Jul 25, 2024
Hey, I'm getting this error after running:
pip install -r requirements.txt -r requirements-dev.txt5 remaining items
shivam-daksh commentedon Aug 2, 2024
HI @MisRob, I've found two ways to fix this issue:
1. Replacing
<KExternalLink/>component with<ActionLink/>. This is same as used in this navigation drawer:The result after:
2. Placing the
KIconButtoncomponent as child inKExternalLinkcomponent.The Result after:
Which one should I proceed with?
MisRob commentedon Aug 2, 2024
Hi @shivam-daksh, thanks, looking good! Please go with option (1) - that's the standard way.
shivam-daksh commentedon Aug 2, 2024
Hi @MisRob, It's done. Check this:
Screen.Recording.2024-08-02.at.2.42.09.PM.mov
MisRob commentedon Aug 2, 2024
@shivam-daksh overall looks like an expected behavior when it comes to appearance of links, so that's good! However I believe there's still some work in regard to links targets, right? I don't think they should all lead to learningequality.org. Their original targets need to be preserved.
shivam-daksh commentedon Aug 2, 2024
Hi @MisRob , Check now:
Screen.Recording.2024-08-02.at.3.10.34.PM.mov
MisRob commentedon Aug 2, 2024
Yes, that looks like expected behavior @shivam-daksh
shivam-daksh commentedon Aug 2, 2024
Thankyou @MisRob for reviewing, Should I send merge request now? If yes, is there anything I've to do?
MisRob commentedon Aug 2, 2024
@shivam-daksh Yes, you can open a pull request and we will then review it. We can't recognize everything from the recording, even though it definitely looks like a good start.
MisRob commentedon Aug 2, 2024
@shivam-daksh Just follow the pull request template and provide information that you can - that's all :)
shivam-daksh commentedon Aug 2, 2024
Thank you @MisRob, I've sent the merge request. #4622
ozer550 commentedon Aug 6, 2024
The links are working as expected and I see the necessary pop-out icons too altho I came across instance where we do not have a space after the period here.
shivam-daksh commentedon Aug 6, 2024
Hi @ozer550 , thank you for identifying this. Now, I've fixed it by adding a space. Please review it again and let me know if there are any other changes required.
Insert space after period in Settings>Storage page
Merge pull request #4622 from shivam-daksh/fix-issue-#4606
rtibbles commentedon Aug 9, 2024
Fixed in #4622