Replace `TextTruncatorCss` with `KTextTruncator` #12215

jasonmokk · 2024-05-27T21:13:11Z

Summary

This PR removes all references to TextTruncatorCss from the codebase in favor of KTextTruncator.

References

Closes #12211

Reviewer guidance

…

Testing checklist

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
Critical and brittle code paths are covered by unit tests

PR process

PR has the correct target branch and milestone
PR has 'needs review' or 'work-in-progress' label
If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
If this is an important user-facing change, PR or related issue has a 'changelog' label
If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

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

kolibri/core/assets/src/core-app/apiSpec.js

MisRob · 2024-05-30T09:02:10Z

kolibri/plugins/coach/assets/src/views/home/HomePage/ItemProgressDisplay.vue

@@ -37,15 +37,15 @@
 <script>

  import { validateLinkObject } from 'kolibri.utils.validators';
-  import TextTruncatorCss from 'kolibri.coreVue.components.TextTruncatorCss';
+  import KTextTruncator from 'kolibri-design-system/lib/KTextTruncator';


Same as my previous comment, I think all imports like this can be removed.

I tried removing all these imports, but it ends up causing a ReferenceError in the tests as seen below. I've updated the PR to only remove the import/definition from apiSpecs.js, is this okay?

I see, hm I don't understand why it doesn't work, but looking at the Kolibri codebase, it's the case for more components. I will look at it later. So I think there's no need to be dealing with this in the scope of this pull request.

@jasonmokk Actually one question - when you removed the import statement, did you also remove KTextTruncator from components on line 48?

No, I did not. I just double checked and still found tests are failing when I remove just the import statement while keeping KTextTruncator in components.

If you don't clean it from components and remove import only then components don't know where to reach for it and that's most likely the cause of the error. So both need to be cleaned up (and that should be fine because KTextTruncator should be globally available for use in all Kolibri components immediately without any extra steps needed). Apologies for my first comment, I wasn't explicit about this and assumed you removed both places. Let me know if this helps.

Ah I see. I tried removing KTextTruncator from components along with the import removal, but unfortunately I am still getting the same ReferenceError when running yarn run test. Perhaps this is a larger issue?

Tests are passing on my localhost after this cleanup. Could you double-check if you removed all places before running tests?

I noticed the most recent commit @jasonmokk and thanks for following-up. The tests are still failing, possibly because one last place in the BaseCard, line 77.

Got it, I've updated it and I believe everything should be passing now.

MisRob · 2024-05-30T09:03:21Z

Thanks @jasonmokk for your contribution. I left some first feedback.

github-actions · 2024-05-30T09:34:25Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-.pex
Windows Installer (EXE)	kolibri-0.17.0a0.dev0+git.2.g9ea52b05-windows-setup-unsigned.exe
Debian Package	kolibri_0.17.0a0.dev0+git.2.g9ea52b05-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.17.0a0.dev0+git.2.g9ea52b05-0.4.2.dmg
Android Package (APK)	kolibri-0.17.0a0.dev0+git.2.g9ea52b05-0.1.3-debug.apk
TAR file	kolibri-0.17.0a0.dev0+git.2.g9ea52b05.tar.gz
WHL file	kolibri-0.17.0a0.dev0+git.2.g9ea52b05-py2.py3-none-any.whl

ghagevaibhav

Hey I came on this issue to solve it, but seen it solved I decided to review the changes and In think everything look's great overall.

MisRob · 2024-06-13T08:44:19Z

kolibri/plugins/learn/assets/src/views/cards/BaseCard.vue

@@ -75,8 +74,7 @@
  export default {
    name: 'BaseCard',
    components: {
-      TextTruncatorCss,
-      CardLink,
+      KTextTruncator,


This will need to be removed as well

MisRob · 2024-06-13T08:46:06Z

Also linting check is failing, you can follow this section of the developer documentation to resolve that.

Remove from apispecs only Remove KTextTruncator from components [pre-commit.ci lite] apply automatic fixes Linting fixup minor fixup

MisRob

All is looking good to me. Thank you and congrats to your first contribution @jasonmokk!

github-actions bot added APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: medium labels May 27, 2024

jasonmokk marked this pull request as ready for review May 27, 2024 21:14

MisRob requested review from LianaHarris360 and AllanOXDi May 30, 2024 08:59

MisRob reviewed May 30, 2024

View reviewed changes

kolibri/core/assets/src/core-app/apiSpec.js Outdated Show resolved Hide resolved

MisRob reviewed May 30, 2024

View reviewed changes

jasonmokk force-pushed the remove-text-truncator-css branch from 219135c to 92cc3a8 Compare May 31, 2024 17:31

ghagevaibhav reviewed Jun 1, 2024

View reviewed changes

MisRob self-assigned this Jun 7, 2024

MisRob reviewed Jun 13, 2024

View reviewed changes

jasonmokk force-pushed the remove-text-truncator-css branch from f994ce8 to c125ddc Compare June 17, 2024 18:17

jasonmokk force-pushed the remove-text-truncator-css branch 2 times, most recently from e3e9de7 to 8f2c3ff Compare June 17, 2024 18:27

Replace TextTruncatorCss with KTextTruncator

5c68c69

Remove from apispecs only Remove KTextTruncator from components [pre-commit.ci lite] apply automatic fixes Linting fixup minor fixup

jasonmokk force-pushed the remove-text-truncator-css branch from 8f2c3ff to 5c68c69 Compare June 17, 2024 18:30

MisRob approved these changes Jun 18, 2024

View reviewed changes

MisRob merged commit d4993f5 into learningequality:develop Jun 18, 2024
31 checks passed

jasonmokk deleted the remove-text-truncator-css branch November 12, 2024 03:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `TextTruncatorCss` with `KTextTruncator` #12215

Replace `TextTruncatorCss` with `KTextTruncator` #12215

jasonmokk commented May 27, 2024 •

edited

Loading

MisRob May 30, 2024

jasonmokk May 31, 2024 •

edited

Loading

MisRob Jun 3, 2024

MisRob Jun 3, 2024

jasonmokk Jun 4, 2024 •

edited

Loading

MisRob Jun 4, 2024

jasonmokk Jun 6, 2024 •

edited

Loading

MisRob Jun 10, 2024

MisRob Jun 13, 2024

jasonmokk Jun 17, 2024

MisRob commented May 30, 2024

github-actions bot commented May 30, 2024 •

edited

Loading

ghagevaibhav left a comment

MisRob Jun 13, 2024

MisRob commented Jun 13, 2024

MisRob left a comment

Replace TextTruncatorCss with KTextTruncator #12215

Replace TextTruncatorCss with KTextTruncator #12215

Conversation

jasonmokk commented May 27, 2024 • edited Loading

Summary

References

Reviewer guidance

Testing checklist

PR process

Reviewer checklist

Choose a reason for hiding this comment

jasonmokk May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmokk Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmokk Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisRob commented May 30, 2024

github-actions bot commented May 30, 2024 • edited Loading

ghagevaibhav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MisRob commented Jun 13, 2024

MisRob left a comment

Choose a reason for hiding this comment

Replace `TextTruncatorCss` with `KTextTruncator` #12215

Replace `TextTruncatorCss` with `KTextTruncator` #12215

jasonmokk commented May 27, 2024 •

edited

Loading

jasonmokk May 31, 2024 •

edited

Loading

jasonmokk Jun 4, 2024 •

edited

Loading

jasonmokk Jun 6, 2024 •

edited

Loading

github-actions bot commented May 30, 2024 •

edited

Loading