Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

long title button overlap resolved #11158

Conversation

GarvitSinghal47
Copy link
Contributor

@GarvitSinghal47 GarvitSinghal47 commented Aug 27, 2023

Fixes #11086

Summary

changes the default old breakpoint for the numBarActions to more suitable one.
also added a filter of text truncation on long title for different breakpoint to prevent button issue as the size of the text content increases.

After :

Transcripts.-.MIT.Blossoms.webm

References

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

@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: small labels Aug 27, 2023
@MisRob MisRob requested a review from LianaHarris360 August 29, 2023 07:38
@MisRob MisRob added the TODO: needs review Waiting for review label Aug 29, 2023
Copy link
Member

@LianaHarris360 LianaHarris360 left a comment

Choose a reason for hiding this comment

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

Hi @GarvitSinghal47, thank you so much for working on this. It looks good, I just had a few comments to add in. Also, in the above References section, if you add in the line Fixes #11086, once this pull request is merged, it will automatically close that issue!

@@ -419,9 +434,9 @@
},
numBarActions() {
let maxSize = 1;
if (this.windowBreakpoint === 1) {
if (this.windowBreakpoint === 1 || this.windowBreakpoint === 2) {
Copy link
Member

Choose a reason for hiding this comment

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

This if statement could instead check if this.windowBreakpoint is less than or equal to 2 as a small refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah that will not work as when the size of breakpoint will go less than 1 we want to show only one icon the refactor you are saying will make two icon to show in that case but originally intended was 1 .

we can change it to get value greater than equal to 1 or less than equal to 2 but i think that will not be of any use.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I understand it now, I overlooked that detail before :)

/>
<TextTruncatorCss
v-else
:text="resourceTitle | truncateText(70)"
Copy link
Member

Choose a reason for hiding this comment

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

More out of curiosity, I'm wondering how the numbers 50 and 70 decided on? In the included picture, the windowBreakpoint is 6, this title has two more characters to display, and seems like it should fit, but it is truncated instead.
Screenshot 2023-08-30 at 2 05 31 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have just added by manually applying it by decreasing the size of window to various breakpoints for large screen when it is greater than 2 breakpoint means till 3 to 6 i have make 70.

it will show good in case of 6 breakpoint but when we take it to 3 or 4 any size of text above or about 75 can create problem of button overlap again.

we can also change it for breakpoint 5 and 6 as well seperately like about 80 to 85 (a it will be safe) but the inital thinking of mine is to cover approx 50-60 % of the header by title in case of breakpoint 5 and 6 .

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the explanation, that makes sense to me! Now that all the tests pass, this should be good to merge in!

@MisRob
Copy link
Member

MisRob commented Aug 31, 2023

I'd like to add that tests from kolibri/plugins/learn/assets/test/views/learning-activity-bar.spec.js are failing and will need to be fixed/adjusted

@LianaHarris360
Copy link
Member

Thanks for catching that @MisRob. Looks like a problem with value.length being undefined within the filter truncateText

console.error
      TypeError: Cannot read properties of undefined (reading 'length')
          at truncateText (/home/runner/work/kolibri/kolibri/kolibri/plugins/learn/assets/src/views/LearningActivityBar.vue:4519:19)

More info about Kolibri tests can be found here.

@LianaHarris360 LianaHarris360 merged commit 846471f into learningequality:release-v0.16.x Sep 5, 2023
@GarvitSinghal47 GarvitSinghal47 deleted the long_content_title branch September 7, 2023 05:04
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long content titles cause issues with learning activity bar buttons
3 participants