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

Issue-450: Made KTextTruncator #464

Merged

Conversation

muditchoudhary
Copy link
Contributor

@muditchoudhary muditchoudhary commented Oct 10, 2023

Description

This PR creates a KTextTruncator component that will be used to truncate texts and this PR creates a ktexttruncator.vue for the docs of the KTextTruncator component.

Issue addressed

Addresses #450

Before/after screenshots

Steps to test

  1. Go to docs/pages/playground.vue
  2. Add the KTextTruncator
<KTextTruncator text="Option (1) is simpler and sufficient as the first step, so we recommend starting that way. On certain occasions, it may be necessary to test an update in a product. If you're a contributor, you are welcome to try (2) as well, but typically we do not require contributors to do so, or we will provide support if needed." :maxLines="1" />
  1. Run the server and visit http://localhost:4000/playground
    yarn dev

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change has been added to the changelog

Reviewer guidance

  • Is the code clean and well-commented?
  • Are there tests for this change?
  • Are all UI components LTR and RTL compliant (if applicable)?
  • Add other things to check for here

Comments

@muditchoudhary
Copy link
Contributor Author

muditchoudhary commented Oct 10, 2023

@MisRob I tried to find any internal styling that is causing the ellipsis to render a little forward from the last word. But I could not find out what actually causing the issue.

Although the KTextTruncator works fine in the playground and other places in Kolibri
For example on the TopicsHeader.vue title.

268653487-5804d9b8-d819-420c-83a6-ac84b14df4c9

@MisRob MisRob changed the base branch from develop to main October 12, 2023 13:06
@MisRob MisRob changed the base branch from main to develop October 12, 2023 13:06
@MisRob
Copy link
Member

MisRob commented Oct 12, 2023

Thank you @muditchoudhary! We will have a look. Could you please rebase on top of the main branch and then retarget the PR there? Alternatively, you can let me know and can do so.

@muditchoudhary muditchoudhary force-pushed the issue-450-add-KTextTruncator branch from 5356f7a to 533a091 Compare October 12, 2023 16:19
@muditchoudhary
Copy link
Contributor Author

Thank you @muditchoudhary! We will have a look. Could you please rebase on top of the main branch and then retarget the PR there? Alternatively, you can let me know and can do so.

@MisRob I rebase successfully to the main. Hope it would be good to test.

@muditchoudhary muditchoudhary changed the base branch from develop to main October 12, 2023 16:24
@MisRob
Copy link
Member

MisRob commented Oct 13, 2023

Great, thank you @muditchoudhary

@MisRob MisRob self-requested a review October 13, 2023 07:27
lib/KTextTruncator.vue Outdated Show resolved Hide resolved
Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @muditchoudhary, code is looking good overall. I left few smaller notes. Also, could you please fix linting check? See package.json for linting commands.

As soon as this is resolved, I will try it out in Kolibri and see if I can observe the issue with the space between ellipsis and text that you mentioned some time ago. In any case, I think this PR won't be a culprit and should be good to go soon.

@MisRob
Copy link
Member

MisRob commented Oct 16, 2023

I'll take care of the changelog check myself before we merge this, so no worries about that one

@muditchoudhary muditchoudhary force-pushed the issue-450-add-KTextTruncator branch from 533a091 to ea0135b Compare October 17, 2023 15:59
@muditchoudhary
Copy link
Contributor Author

Thanks @muditchoudhary, code is looking good overall. I left few smaller notes. Also, could you please fix linting check? See package.json for linting commands.

As soon as this is resolved, I will try it out in Kolibri and see if I can observe the issue with the space between ellipsis and text that you mentioned some time ago. In any case, I think this PR won't be a culprit and should be good to go soon.

Welcome! I fixed the minor issues you mentioned in the review.

@muditchoudhary muditchoudhary marked this pull request as ready for review October 18, 2023 04:04
lib/KTextTruncator.vue Outdated Show resolved Hide resolved
Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@muditchoudhary Great work and engagement during the review process, thank you.

I tried to find any internal styling that is causing the ellipsis to render a little forward from the last word. But I could not find out what actually causing the issue.

I just tested the place you mentioned in #450 in this regard and everything is okay.

With that, I also confirm I was able to use KTextTruncator in the latest Kolibri successfully locally so all good!

Leaving only one minor non-blocking note.

- Removed the old condition that checks for nuxt client rendering
- add this condition because window object is not defined in server rendering
- it is present only in client (browser).
@muditchoudhary muditchoudhary force-pushed the issue-450-add-KTextTruncator branch from ea0135b to 64d66d0 Compare October 20, 2023 13:46
@muditchoudhary
Copy link
Contributor Author

@MisRob I added the condition you told me and tested it out, and there is no issue.

Also thanks for being helpful in the review.

@MisRob MisRob merged commit 13e5395 into learningequality:main Oct 23, 2023
7 checks passed
@MisRob MisRob mentioned this pull request Oct 23, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants