Fix to issue #4606 : Links leading outside Studio need to have a pop out icon #4622

shivam-daksh · 2024-08-02T12:45:06Z

Summary

Description of the change(s) you made

This merge request addresses the issue where external links in Studio did not have a visual indicator to distinguish them from internal links. The affected links also did not open in a new tab by default. To resolve this, the KExternalLink component has been replaced with the ActionLink component, and a CSS class inline-icon has been added to provide the necessary visual indication.

Manual verification steps performed

Step 1 : Replaced instances of KExternalLink with ActionLink to ensure consistent behavior and appearance across the application.
Step 2 : Added the inline-icon CSS class to the ActionLink component to display a pop-out icon for adjusting the position.
...

Screenshots (if applicable)

Screen.Recording.2024-08-02.at.3.10.34.PM.mov

Affected Areas

**In Settings -> page -> Account -> Index.vue : ** Updated the "API documentation" link to use the ActionLink component with the inline-icon class.
In Settings -> page -> UsingStudio -> index.vue : Updated the "User Guide", "bestPractice6", "issueLink1", "issuesPageLink" link to use the ActionLink component replacing KExternalLink component and adding the inline-icon class rep.

Reviewer guidance

How can a reviewer test these changes?

Go to Studio then Settings.
Check all the links leading outside the studio have pop-out icon (open in new tab) in all three tabs.

Contributor's Checklist

PR process:

If this is an important user-facing change, PR or related issue the CHANGELOG label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later time
If this includes an internal dependency change, a link to the diff is provided
The docs label has been added if this introduces a change that needs to be updated in the user docs?
If any Python requirements have changed, the updated requirements.txt files also included in this PR
Opportunities for using Google Analytics here are noted
Migrations are safe for a large db

Studio-specifc:

All user-facing strings are translated properly
The notranslate class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)
All UI components are LTR and RTL compliant
Views are organized into pages, components, and layouts directories as described in the docs
Users' storage used is recalculated properly on any changes to main tree files
If there new ways this uses user data that needs to be factored into our Privacy Policy, it has been noted.

Testing:

Code is clean and well-commented
Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical user journeys are covered by Gherkin stories
Any new interactions have been added to the QA Sheet
Critical and brittle code paths are covered by unit tests

Reviewer's Checklist

This section is for reviewers to fill out.

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

MisRob

I'm sorry @shivam-daksh but we don't want to replace KExternalLink by ActionLink as ActionLink is supposed to be gradually replaced by Kolibri Design System components. That won't be done in scope of #4606, however we don't want move in the opposite direction.

I left a pretty detailed instructions in the "Guidance" section of #4606 that will help you to resolve it while keeping KExternalLink. Please try to follow it or ask questions here if there's anything that's not clear and I will make sure to clear any doubts.

docker-compose.yml

MisRob · 2024-08-03T13:10:31Z

@shivam-daksh In addition to the guidance in the issue, also see https://design-system.learningequality.org/kexternallink/. Apologies, I realize it'd be better to link it sooner.

…op to true

shivam-daksh · 2024-08-04T00:38:13Z

Hi @MisRob, as you said, I've changed the ActionLink back to KExternalLink adding a prop openInNewTab to "true" . This has automatically added the redirect icon. Also, I've reverted the changes from docker-compose.yml which was done automatically by Prettier extension in my IDE.

Resolved

MisRob · 2024-08-05T08:01:27Z

Thanks @shivam-daksh! The latest changes are aligned with the guidance and look well overall to me. I currently have a queue of several larger PRs so I will loop in my colleagues for more detailed review and manual confirmation. @ozer550 and @AllanOXDi would you please give it a look and test when you have a moment? Here's the issue #4606. Thank you.

AllanOXDi · 2024-08-06T19:38:26Z

Hi @shivam-daksh Thanks for woking on this . links leading outside Studio now have a pop out icon. I am leaving a few clean up comments

contentcuration/contentcuration/frontend/settings/pages/UsingStudio/index.vue

AllanOXDi · 2024-08-06T20:14:46Z

contentcuration/contentcuration/frontend/settings/pages/UsingStudio/index.vue

      href="https://github.com/learningequality/studio/issues"
      :text="$tr('issuesPageLink')"
-      target="_blank"
+      openInNewTab="true"


The openInNewTab="true" is used correctly. However, in Vue, boolean props can be passed without a value. So it could be simplified to just openInNewTab. or :openInNewTab="true"

Thank you for letting me know. I've changed it to simplified.

AllanOXDi · 2024-08-06T20:15:24Z

contentcuration/contentcuration/frontend/settings/pages/Storage/index.vue

-</style>
+  .kexternal-redirect{
+    margin-left: -8px;
+    display: inline !important;


The !important property might not be necessary if there are no conflicting styles. It's generally best to avoid !important unless absolutely necessary. In fact, if you use the !important property , it will override all previous styling rules for that specific property on that element :)

And display property was indeed unnecessary, so I've removed it.

AllanOXDi

Hi @shivam-daksh, thanks for your hard work on this. The work looks good! Let's just make sure that the linting checks are passing, and then we can merge the changes. You can run yarn run lint-frontend:format

shivam-daksh · 2024-08-08T13:11:13Z

Hi @shivam-daksh, thanks for your hard work on this. The work looks good! Let's just make sure that the linting checks are passing, and then we can merge the changes. You can run yarn run lint-frontend:format

Hey @AllanOXDi, I'm getting this error after running yarn run lint-frontend:format

(.venv) shivam@Shivam-2 studio % yarn run lint-frontend:format
yarn run v1.22.22
$ yarn run lint-frontend --write
$ kolibri-tools lint --pattern 'contentcuration/contentcuration/frontend/**/*.{js,vue,scss,less,css}' --ignore '**/dist/**,**/node_modules/**,**/static/**' --write
Error: ENOENT: no such file or directory, open '/Volumes/Macintosh HD - Data/Space A/Open-Source/studio/kolibri/core/assets/src/core-app/apiSpec.js'
    at Object.openSync (node:fs:590:3)
    at Object.readFileSync (node:fs:458:35)
    at specModule (/Volumes/Macintosh HD - Data/Space A/Open-Source/studio/node_modules/kolibri-tools/lib/apiSpecExportTools.js:37:26)
    at Object.<anonymous> (/Volumes/Macintosh HD - Data/Space A/Open-Source/studio/node_modules/kolibri-tools/lib/apiSpecExportTools.js:103:13)
    at Module._compile (node:internal/modules/cjs/loader:1198:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1252:10)
    at Module.load (node:internal/modules/cjs/loader:1076:32)
    at Function.Module._load (node:internal/modules/cjs/loader:911:12)
    at Module.require (node:internal/modules/cjs/loader:1100:19)
    at require (node:internal/modules/cjs/helpers:119:18) {
  errno: -2,
  syscall: 'open',
  code: 'ENOENT',
  path: '/Volumes/Macintosh HD - Data/Space A/Open-Source/studio/kolibri/core/assets/src/core-app/apiSpec.js'
}
✨  Done in 48.35s.

MisRob · 2024-08-09T09:01:56Z

@shivam-daksh We figured out that what you're seeing may be just confusing output of the command. Running yarn run lint-frontend:format likely changed some of your local files. Do you have some uncommited changes after running it? If yes, please commit them and that should help.

shivam-daksh · 2024-08-09T09:14:48Z

@shivam-daksh We figured out that what you're seeing may be just confusing output of the command. Running yarn run lint-frontend:format likely changed some of your local files. Do you have some uncommited changes after running it? If yes, please commit them and that should help.

Hi @MisRob, Initially, my working tree was clean. But when I run that command a single change occured in a file that I commited. My working tree was clean and I ran the command yarn run lint-frontend:format and again same error occured but without changing any files.
But I'm thinking that it is missing a file /Volumes/Macintosh HD - Data/Space A/Open-Source/studio/kolibri/core/assets/src/core-app/apiSpec.js which is not present. Actually, there is no such directory as kolibri in studio. It is actually a totally different repository. Kolibri

(.venv) shivam@Shivam-2 studio % git status
On branch fix-issue-#4606
nothing to commit, working tree clean
(.venv) shivam@Shivam-2 studio % yarn run lint-frontend:format
yarn run v1.22.22
$ yarn run lint-frontend --write
$ kolibri-tools lint --pattern 'contentcuration/contentcuration/frontend/**/*.{js,vue,scss,less,css}' --ignore '**/dist/**,**/node_modules/**,**/static/**' --write
Error: ENOENT: no such file or directory, open '/Volumes/Macintosh HD - Data/Space A/Open-Source/studio/kolibri/core/assets/src/core-app/apiSpec.js'
    at Object.openSync (node:fs:590:3)
    at Object.readFileSync (node:fs:458:35)
    at specModule (/Volumes/Macintosh HD - Data/Space A/Open-Source/studio/node_modules/kolibri-tools/lib/apiSpecExportTools.js:37:26)
    at Object.<anonymous> (/Volumes/Macintosh HD - Data/Space A/Open-Source/studio/node_modules/kolibri-tools/lib/apiSpecExportTools.js:103:13)
    at Module._compile (node:internal/modules/cjs/loader:1198:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1252:10)
    at Module.load (node:internal/modules/cjs/loader:1076:32)
    at Function.Module._load (node:internal/modules/cjs/loader:911:12)
    at Module.require (node:internal/modules/cjs/loader:1100:19)
    at require (node:internal/modules/cjs/helpers:119:18) {
  errno: -2,
  syscall: 'open',
  code: 'ENOENT',
  path: '/Volumes/Macintosh HD - Data/Space A/Open-Source/studio/kolibri/core/assets/src/core-app/apiSpec.js'
}
Kolibri Linter: Rewriting a prettier version of contentcuration/contentcuration/frontend/settings/pages/Storage/index.vue
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
(.venv) shivam@Shivam-2 studio % git status
On branch fix-issue-#4606
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   contentcuration/contentcuration/frontend/settings/pages/Storage/index.vue

no changes added to commit (use "git add" and/or "git commit -a")
(.venv) shivam@Shivam-2 studio % git add .
(.venv) shivam@Shivam-2 studio % git commit -m "yarn lint-frontend:format changes"          
[fix-issue-#4606 922b49e08] yarn lint-frontend:format changes
 1 file changed, 1 insertion(+)
(.venv) shivam@Shivam-2 studio % yarn run lint-frontend:format                    
yarn run v1.22.22
$ yarn run lint-frontend --write
$ kolibri-tools lint --pattern 'contentcuration/contentcuration/frontend/**/*.{js,vue,scss,less,css}' --ignore '**/dist/**,**/node_modules/**,**/static/**' --write
Error: ENOENT: no such file or directory, open '/Volumes/Macintosh HD - Data/Space A/Open-Source/studio/kolibri/core/assets/src/core-app/apiSpec.js'
    at Object.openSync (node:fs:590:3)
    at Object.readFileSync (node:fs:458:35)
    at specModule (/Volumes/Macintosh HD - Data/Space A/Open-Source/studio/node_modules/kolibri-tools/lib/apiSpecExportTools.js:37:26)
    at Object.<anonymous> (/Volumes/Macintosh HD - Data/Space A/Open-Source/studio/node_modules/kolibri-tools/lib/apiSpecExportTools.js:103:13)
    at Module._compile (node:internal/modules/cjs/loader:1198:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1252:10)
    at Module.load (node:internal/modules/cjs/loader:1076:32)
    at Function.Module._load (node:internal/modules/cjs/loader:911:12)
    at Module.require (node:internal/modules/cjs/loader:1100:19)
    at require (node:internal/modules/cjs/helpers:119:18) {
  errno: -2,
  syscall: 'open',
  code: 'ENOENT',
  path: '/Volumes/Macintosh HD - Data/Space A/Open-Source/studio/kolibri/core/assets/src/core-app/apiSpec.js'
}
✨  Done in 44.30s.
(.venv) shivam@Shivam-2 studio % git status
On branch fix-issue-#4606
nothing to commit, working tree clean

MisRob · 2024-08-09T09:21:30Z

@shivam-daksh Thanks for following up. So the first step now - can you push the commited file just to see if that resolves the check on this PR?

MisRob · 2024-08-09T09:40:38Z

@shivam-daksh Oh you already did, sorry I missed that! Re-running the checks now.

MisRob · 2024-08-09T09:43:40Z

@shivam-daksh Linting passes now ;) So all good in the scope of this PR. Thanks for reporting the outputs, we took a note of it. Also thanks @AllanOXDi and @Pursottam6003, I think it's in good shape now thanks to your feedback. I skimmed briefly through the latest changes too and as soon as all checks pass, seems ready for merge to me.

MisRob · 2024-08-09T09:44:03Z

Congrats to your first contribution @shivam-daksh and thanks for following-up!

AllanOXDi · 2024-08-09T09:55:59Z

Well done 🎉 @shivam-daksh

shivam-daksh · 2024-08-09T09:58:22Z

Thank you, @MisRob, for your guidance throughout this process!
I appreciate your patience and support in helping me navigate my first contribution. I'm glad we were able to resolve the issues, and it feels great to see everything in good shape. Also, a big thanks to @AllanOXDi, @ozer550 and @Pursottam6003 for their valuable feedback.
Looking forward to contributing more and learning along the way. Thanks again for the encouragement and for making this a smooth experience!

shivam-daksh added 3 commits August 2, 2024 13:35

found two ways to fix issue

f02a8dd

Issue fixed by replacing KExternalLink Component with ActionLink

638d49d

Preserve Link Targets

e71cc90

shivam-daksh mentioned this pull request Aug 2, 2024

Links leading outside Studio need to have a pop out icon #4606

Closed

MisRob previously requested changes Aug 3, 2024

View reviewed changes

MisRob reviewed Aug 3, 2024

View reviewed changes

docker-compose.yml Outdated Show resolved Hide resolved

shivam-daksh added 3 commits August 4, 2024 04:35

docker-compose.yml changes reverted

957f872

ActionLink is changed back to KExternalLink and added openInNewTab pr…

fe70b34

…op to true

fixed minor changes and working fine

854790b

shivam-daksh requested a review from MisRob August 4, 2024 00:35

MisRob requested review from ozer550 and AllanOXDi and removed request for MisRob August 5, 2024 08:00

Insert space after period in Settings>Storage page

c8ce5ca

AllanOXDi requested changes Aug 6, 2024

View reviewed changes

Cleaning and simplifying the code

ba7eb92

shivam-daksh requested a review from AllanOXDi August 7, 2024 00:27

Remove margin-left from KExternalLink

ba6a0bf

shivam-daksh requested a review from MisRob August 7, 2024 12:42

added margin-left in Settings->About Studio's external links

64c06a3

AllanOXDi approved these changes Aug 8, 2024

View reviewed changes

yarn lint-frontend:format changes

922b49e

AllanOXDi merged commit 4570288 into learningequality:unstable Aug 9, 2024
13 checks passed

akolson mentioned this pull request Aug 13, 2024

Bulk Edit Release #4635

Merged

shivam-daksh mentioned this pull request Aug 21, 2024

Add $darken_ Utility Functions for Darkening Palette Colors learningequality/kolibri-design-system#728

Merged

9 tasks

akolson mentioned this pull request Oct 1, 2024

Release v2024.10.02 #4776

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to issue #4606 : Links leading outside Studio need to have a pop out icon #4622

Fix to issue #4606 : Links leading outside Studio need to have a pop out icon #4622

shivam-daksh commented Aug 2, 2024

MisRob left a comment •

edited

Loading

MisRob commented Aug 3, 2024

shivam-daksh commented Aug 4, 2024 •

edited

Loading

MisRob commented Aug 5, 2024

AllanOXDi commented Aug 6, 2024

AllanOXDi Aug 6, 2024 •

edited

Loading

shivam-daksh Aug 7, 2024

AllanOXDi Aug 6, 2024

shivam-daksh Aug 7, 2024

AllanOXDi left a comment

shivam-daksh commented Aug 8, 2024

MisRob commented Aug 9, 2024

shivam-daksh commented Aug 9, 2024

MisRob commented Aug 9, 2024

MisRob commented Aug 9, 2024

MisRob commented Aug 9, 2024

MisRob commented Aug 9, 2024

AllanOXDi commented Aug 9, 2024

shivam-daksh commented Aug 9, 2024

Fix to issue #4606 : Links leading outside Studio need to have a pop out icon #4622

Fix to issue #4606 : Links leading outside Studio need to have a pop out icon #4622

Conversation

shivam-daksh commented Aug 2, 2024

Summary

Description of the change(s) you made

Manual verification steps performed

Screenshots (if applicable)

Affected Areas

Reviewer guidance

How can a reviewer test these changes?

Contributor's Checklist

Reviewer's Checklist

This section is for reviewers to fill out.

MisRob left a comment • edited Loading

Choose a reason for hiding this comment

MisRob commented Aug 3, 2024

shivam-daksh commented Aug 4, 2024 • edited Loading

MisRob commented Aug 5, 2024

AllanOXDi commented Aug 6, 2024

AllanOXDi Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

shivam-daksh Aug 7, 2024

Choose a reason for hiding this comment

AllanOXDi Aug 6, 2024

Choose a reason for hiding this comment

shivam-daksh Aug 7, 2024

Choose a reason for hiding this comment

AllanOXDi left a comment

Choose a reason for hiding this comment

shivam-daksh commented Aug 8, 2024

MisRob commented Aug 9, 2024

shivam-daksh commented Aug 9, 2024

MisRob commented Aug 9, 2024

MisRob commented Aug 9, 2024

MisRob commented Aug 9, 2024

MisRob commented Aug 9, 2024

AllanOXDi commented Aug 9, 2024

shivam-daksh commented Aug 9, 2024

MisRob left a comment •

edited

Loading

shivam-daksh commented Aug 4, 2024 •

edited

Loading

AllanOXDi Aug 6, 2024 •

edited

Loading