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

Remove Internet Explorer 11 specific logic from KTextTruncator #652

Merged
merged 6 commits into from
Jun 18, 2024

Conversation

Nivas7
Copy link
Contributor

@Nivas7 Nivas7 commented Jun 8, 2024

Description

In summary, to remove IE11 support from the KTextTruncator component:

Remove the lineHeightIE prop.
Remove the IE11 fallback logic and related styles from the truncate method.

This change, treated as a breaking change, aims to simplify the component by removing unnecessary code for the unsupported IE11 browser. The updated component should be thoroughly tested to ensure no regressions.

Issue addressed

#643

Addresses #652

Before/after screenshots

Before
image

After
image

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?

After review

  • The changelog item has been pasted to the CHANGELOG.md

Comments

@MisRob MisRob self-requested a review June 10, 2024 08:30
@MisRob MisRob self-assigned this Jun 10, 2024
@MisRob
Copy link
Member

MisRob commented Jun 10, 2024

Thank you for following up. I think some more cleanup is needed, since

if ('CSS' in window && CSS.supports && CSS.supports('-webkit-line-clamp: 54'))

condition is not needed anymore.

Can you simplify the truncate method's structure to

  if (this.maxLines === 1) {
    ...
  } else {
   ...
}

?

}
// Default return value for when neither condition is met
return {};
Copy link
Member

Choose a reason for hiding this comment

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

This won't be needed, see #652 (comment)

CHANGELOG.md Outdated
- **Components:** KTextTruncator
- **Breaking:** no
- **Impacts a11y:** -
- **Guidance:** Consumers need to ensure the modal is still working correctly.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these changes have anything to do with the modal, possibly result of copy-paste? Please cleanup.

@Nivas7
Copy link
Contributor Author

Nivas7 commented Jun 11, 2024

Thank you for your guidance,can you check a latest commit @MisRob

@MisRob
Copy link
Member

MisRob commented Jun 13, 2024

Thank you @Nivas7, all is looking good to me now. One last thing before merge - please run yarn lint-fix and commit the fixes. This should resolve the linting check failure.

@Nivas7
Copy link
Contributor Author

Nivas7 commented Jun 13, 2024

Thanks for ur support

@Nivas7
Copy link
Contributor Author

Nivas7 commented Jun 17, 2024

Apologize for late i fixed linting errors. @MisRob thinks the code should works fine.

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 @Nivas7, no problem! All is looking good to me (just pushing a small update of changelog). Thanks and congratulations to your first contribution.

@MisRob MisRob merged commit aee12cf into learningequality:develop Jun 18, 2024
8 checks passed
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