Skip to content
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

Replace TextTruncator by TextTruncatorCss #8532

Closed
MisRob opened this issue Oct 25, 2021 · 30 comments
Closed

Replace TextTruncator by TextTruncatorCss #8532

MisRob opened this issue Oct 25, 2021 · 30 comments
Assignees
Labels
DEV: frontend good first issue Self-contained, straightforward, low-complexity help wanted Open source contributors welcome P1 - important Priority: High impact on UX TAG: tech update / debt Change not visible to user

Comments

@MisRob
Copy link
Member

MisRob commented Oct 25, 2021

Observed behavior

In #8464, where a new text truncator component based on CSS, TextTuncatorCss, was introduced to the codebase as an alternative to JS-based TextTruncator to prevent introducing more performance issues similar to ones reported in #8124, it seemed that we agreed on replacing TextTruncator by TextTruncatorCss completely. Alternatively, we could discuss creating KTextTruncator from TextTruncatorCss in KDS and migrating to it.

See @indirectlylit's comment

The CSS implementation degrades gracefully on IE11 and otherwise the behaviors are quite similar, so I would support migrating to it entirely rather than maintaining two parallel implementations.

Shorter-term the main question is cost/risk of migration – if either are high, we might want to use both at least until 0.15. Longer-term once things are consolidated, we could even move it to the design system as KTextTruncator.

Note that as @indirectlylit mentioned here we need to be careful about testing the new truncator properly in regards to the following issues to avoid regressions:

Expected behavior

  • TextTruncatorCss or KTextTruncator is used everywhere and TextTruncator exists no more

User-facing consequences

Performance improvements

@Mohit1345
Copy link

Mohit1345 commented Feb 1, 2023

@MisRob I am interested to contribute over this issue.

@Mohit1345
Copy link

@nucleogenesis can I start to solve this .

@MisRob
Copy link
Member Author

MisRob commented Feb 3, 2023

Hello @Mohit1345, thank you for your interest in contributing. Yes, you're welcome to take this on. Please have a look at our developer documentation if you haven't seen it already. It'd be helpful if you posted a comment here as soon as you start working on it so that we know it's in progress and I will also assign you on that opportunity.

@Pursottam6003
Copy link
Contributor

Hi @MisRob , I am interested in working on this issue. Could you please assign me

@MisRob
Copy link
Member Author

MisRob commented Apr 3, 2023

Hello @Pursottam6003, I'm assigning you, however, if you prefer to work on some other issues you volunteered for, please post here.

@Pursottam6003
Copy link
Contributor

Hello @Pursottam6003, I'm assigning you, however, if you prefer to work on some other issues you volunteered for, please post here.

Sure @MisRob I would be very happy if you could assign me to the following issue currently I noticed it requires minor changes but those changes are required so I created the following issues and awaiting to be assigned

  1. Clear button not visible properly in Learn > Library > Channels selection box Clear button not visible properly in Learn > Library > Channels selection box  #10370

  2. Accesibility issue : Right navigator button > is not working with tab in Library-> Resources-> reading Accesibility issue : Right navigator button > is not working with tab in Library-> Resources-> reading #10371

Thanking You

@MisRob
Copy link
Member Author

MisRob commented Apr 5, 2023

Alright @Pursottam6003, I'm unassigning you from this one and assigning you one of the issues you referenced

@Pursottam6003
Copy link
Contributor

Hii @MisRob

as of now, I am only left with one assigned task which I will complete soon

But I was having a little doubt; from the issue, i had noticed that we used both TextTruncator and TextTruncatorCss and I noticed some pull requests were also merged

so could you please tell me where I need to replace TextTruncator with TextTruncatorCss or KTextTruncator since there will be lots of changes in all files

Screenshot from 2023-04-13 22-41-32

@MisRob
Copy link
Member Author

MisRob commented Apr 14, 2023

Hello @Pursottam6003,

some time ago we decided to get rid of TextTruncator completely everywhere in favour of TextTruncatorCss so I confirm. We know there are more instances. You can check out links in the description of the issue that point to some discussions we had and related issues (not required though).

However, I wouldn't recommend doing it all in one huge pull request. If you could open smaller pull requests, that'd be welcome. It will be reviewed faster and we can clarify issues more easily if you encounter some before you refactor it all. You could, for example, refactor one page or a plugin, depending on the number of TextTruncator instances.

@Pursottam6003
Copy link
Contributor

Hii @MisRob whenever you are free kindly check the PR it's having minor changes if it needs certain modification kindly do let me know :)

Thank You

@MisRob
Copy link
Member Author

MisRob commented Apr 28, 2023

The PR #10518 removes TextTruncator in favor of TextTruncatorCss at some places but as agreed here, it's being replaced gradually so leaving this issue open until we finish it.

MisRob added a commit that referenced this issue Apr 28, 2023
Fixed Update #8532 Replace TextTruncator with TextTruncatorCss
@Pursottam6003
Copy link
Contributor

The PR #10518 removes TextTruncator in favor of TextTruncatorCss at some places but as agreed here, it's being replaced gradually so leaving this issue open until we finish it.

Sure @MisRob I will try to update wholeTextTruncator with TextTruncatorCss and ask doubts whenever I need guidance :)

rtibbles added a commit that referenced this issue May 3, 2023
@Jaspreet-singh-1032
Copy link
Contributor

@MisRob, Please assign this issue to me.
Thanks!

@MisRob
Copy link
Member Author

MisRob commented Jul 7, 2023

@Jaspreet-singh-1032 Done, thank you! (assigning myself too as a contact person, but won't work on it)

@MisRob MisRob added good first issue Self-contained, straightforward, low-complexity and removed TAG: beginner friendly labels Aug 18, 2023
@MisRob
Copy link
Member Author

MisRob commented Sep 15, 2023

@Jaspreet-singh-1032 There has been no activity for a long time here and you worked on some other contributions so I assume it's not in progress. If that's wrong, you can coordinate with another contributor I just talked to and who might be asking here for an assignment. It shouldn't be a problem since there are many places to work at.

@Jaspreet-singh-1032
Copy link
Contributor

Hi @MisRob, Yes I was not able to work on this due to other tasks. If someone wants to work on it they can go ahead.

@muditchoudhary
Copy link
Contributor

muditchoudhary commented Sep 17, 2023

Hello @MisRob I understand the issue. Could please assign it to me? I'm planning to fix the Coach plugin files first. I'm thinking of having 5 files in one PR for a particular plugin. So that it would be easy to review.
Are 5 files fine in one PR or we should have less?

Also @Pursottam6003 I saw that in the coach plugin you made changes to this file:

  • kolibri/plugins/coach/assets/src/views/plan/LessonResourceSelectionPage/LessonContentCard/index.vue

Are there any files in Coach plugin that you have fixed in your local environment which I'm not aware of? If yes, you can tell me I won't fix them.

Update
I just found out that not all files have TextTruncator used in them. The coach plugin has only one file that @Pursottam6003 has already fixed. I'll find the other files that are not yet fixed, and raise PR for them.

@MisRob
Copy link
Member Author

MisRob commented Sep 18, 2023

@muditchoudhary Thank you. I'm assigning you.

Feel free to go ahead with all the occurrences you've found, I don't think that there's someone with any local work in progress. 5 files is okay. The only important thing is having a reasonable chunk of work to review and test.

Please target your pull request to the develop branch. It'd be helpful if you could specify what places were affected so that our QA team can test easily for regressions (you can just jot down how did you navigate to a place as you're testing it yourself and then paste your notes to the pull request). As soon as you remove the last occurrence, it'd be good to remove TextTruncator.

As you asked for some new issues, I wanted to mention that there's a follow-up work where we'd like to move TextTruncatorCss to our Kolibri Design System. If you'd be interesting in working on that, let me know when the time comes and I can provide more information.

@muditchoudhary
Copy link
Contributor

muditchoudhary commented Sep 18, 2023

@MisRob Only 3 files were left for which I made the changes already in my local env. I would raise 3 different PRs for these 3 files so it would be easy for the QA team to review and communicate with us. I'll make sure in the PR template I'll describe where these changes affect in Kolibri.

As you asked for some new issues, I wanted to mention that there's a follow-up work where we'd like to move TextTruncatorCss to our Kolibri Design System. If you'd be interesting in working on that, let me know when the time comes and I can provide more information.

Sure I would love to work on that. You can provide more information at any time.

@muditchoudhary
Copy link
Contributor

@MisRob Done with making PRs. Please have a review.
Thanks!!

@MisRob
Copy link
Member Author

MisRob commented Sep 19, 2023

Great @muditchoudhary, we will have a look in the next several days. Thank you.

@muditchoudhary
Copy link
Contributor

As you asked for some new issues, I wanted to mention that there's a follow-up work where we'd like to move TextTruncatorCss to our Kolibri Design System. If you'd be interesting in working on that, let me know when the time comes and I can provide more information.

Hello @MisRob since the PRs have merged, now I can work on the follow-up issue you told me about. You can mention me on the issue if it's already created otherwise I think you can tell more about it here. Thanks!

@MisRob
Copy link
Member Author

MisRob commented Sep 22, 2023

Hi @muditchoudhary, great, I was planning to open an issue for that today. If you'd like to, you can familiarize yourself with kolibri-design-system repository meanwhile where you'd be eventually moving the component. Please start here https://github.com/learningequality/kolibri-design-system/blob/develop/CONTRIBUTING.md and try to run the development server. Also, for this particular task, I'd recommend you to study https://github.com/learningequality/kolibri-design-system/blob/develop/dev_docs/03_how_to_update_library.md

These are pretty new contributing guidelines and if you decide to give this a try, I'd be grateful to hear your feedback on the developer documentation.

@MisRob
Copy link
Member Author

MisRob commented Sep 22, 2023

@muditchoudhary This is the issue learningequality/kolibri-design-system#450. Please message us there as soon as you're ready and I will assign you.

@MisRob
Copy link
Member Author

MisRob commented Sep 22, 2023

@muditchoudhary FYI there's another follow-up #11287 but let's keep that for another contributor since you already have some experience and a new issue ready above

@MisRob
Copy link
Member Author

MisRob commented Sep 22, 2023

Thanks so much for completing this work @muditchoudhary and continuing with wonderful contributions

@MisRob MisRob closed this as completed Sep 22, 2023
@MisRob
Copy link
Member Author

MisRob commented Sep 22, 2023

And thanks @Pursottam6003 for starting work on this!

@muditchoudhary
Copy link
Contributor

Thanks so much for completing this work @muditchoudhary and continuing with wonderful contributions

Thanks to you too for helping me and being a really awesome maintainer. I really appreciate your support.

@MisRob
Copy link
Member Author

MisRob commented Oct 2, 2023

I just came around to this old issue by chance #8124. So wanted to mention that this contribution, even though it looks to be simple, should help us with performance quite a lot (didn't do measurements, but pretty sure) @muditchoudhary and @Pursottam6003.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: frontend good first issue Self-contained, straightforward, low-complexity help wanted Open source contributors welcome P1 - important Priority: High impact on UX TAG: tech update / debt Change not visible to user
Projects
None yet
Development

No branches or pull requests

8 participants