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

KTextbox: Support a "clearable" prop to show a clear button #591

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

practicatto
Copy link
Contributor

@practicatto practicatto commented Mar 26, 2024

Description

KTextbox support to show clearable button when clearable prop to true

Issue addressed

Addresses #584

Before/after screenshots

Changelog

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 is described in the changelog section above

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

After review

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

@MisRob MisRob self-assigned this Mar 27, 2024
@practicatto practicatto marked this pull request as ready for review March 28, 2024 22:21
@MisRob MisRob requested review from MisRob, AlexVelezLl and ozer550 and removed request for MisRob April 1, 2024 08:52
@MisRob MisRob removed their assignment Apr 2, 2024
@MisRob MisRob added the TODO: needs review Waiting for review label Apr 2, 2024
Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

Hi! @practicatto! Thanks for your work here, this is looking great! But there are some little things we can improve =)

lib/KTextbox/__tests__/KTextbox.spec.js Outdated Show resolved Hide resolved
lib/KTextbox/__tests__/KTextbox.spec.js Outdated Show resolved Hide resolved
docs/pages/playground.vue Outdated Show resolved Hide resolved
lib/keen/UiTextbox.vue Outdated Show resolved Hide resolved
lib/keen/UiTextbox.vue Outdated Show resolved Hide resolved
lib/keen/UiTextbox.vue Outdated Show resolved Hide resolved
lib/keen/UiTextbox.vue Show resolved Hide resolved
@AlexVelezLl
Copy link
Member

Also, a check is failing because we need to update the Changelog file with a new entry that is the same as the one you put in the PR descripciton.

Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

THanks! @practicatto Just one last thing and we are ready to go!

CHANGELOG.md Show resolved Hide resolved
lib/keen/UiTextbox.vue Show resolved Hide resolved
add tests and fix clear button

remove unnecessary styles and add kiconbutton instead of button

fix tests with component

fix linter

fix borders

add requested changes

add aria label

add link in changelog pr591

add docs

fix lint

fix lint
Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

Thanks for your work @practicatto! Code makes sense and everything seems to be working ok. Thank you!

@AlexVelezLl AlexVelezLl merged commit ff0648c into learningequality:release-v4 Apr 8, 2024
8 checks passed
@practicatto
Copy link
Contributor Author

Thanks for your work @practicatto! Code makes sense and everything seems to be working ok. Thank you!

Thank you for your guidance!! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants