-
Notifications
You must be signed in to change notification settings - Fork 157
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
Links leading outside Studio need to have a pop out icon #4606
Comments
Hey @MisRob , I would like to work on this. |
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 :). |
Thanks @AlexVelezLl for assigning me this task. I've started working. I'm new here, so need little more time to explore. |
Sure! @shivam-daksh No rush 😃. |
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? |
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? |
Hi @MisRob, I've attached the terminal output file along with the screen recording. Please let me know the steps to fix it. Screen.Recording.2024-07-25.at.1.49.44.PM.mov |
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 :) |
Looking at the path to the site packages directory, It looks like you are using python 3.11. Do you have other python versions manager? |
@AlexVelezLl, Which python version I've to use? I've installed: |
@shivam-daksh From your last message, it's not clear to me if this is your system Python or rather Studio environment Python. It's best not to use/update your system python, but rather use python versions manager, such as Have you followed this section step by step and were there any issues with any of the steps? It needs to be |
Thanks @MisRob and @AlexVelezLl, it's working fine 👍. |
Good to hear @shivam-daksh |
HI @MisRob, I've found two ways to fix this issue: 1. Replacing
|
Hi @shivam-daksh, thanks, looking good! Please go with option (1) - that's the standard way. |
Hi @MisRob, It's done. Check this: Screen.Recording.2024-08-02.at.2.42.09.PM.mov |
@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. |
Hi @MisRob , Check now: Screen.Recording.2024-08-02.at.3.10.34.PM.mov |
Yes, that looks like expected behavior @shivam-daksh |
Thankyou @MisRob for reviewing, Should I send merge request now? If yes, is there anything I've to do? |
@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. |
@shivam-daksh Just follow the pull request template and provide information that you can - that's all :) |
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. |
Fix to issue #4606 : Links leading outside Studio need to have a pop out icon
Fixed in #4622 |
🌱 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
<KExternalLink>
's are used and ensure that all links that lead outside of Studio haveopenInNewTab
set to truthy. Relatedly clean uptarget="_blank"
since this is taken care of byopenInNewTab
.<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.The text was updated successfully, but these errors were encountered: