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

removed updation of style while text updates #449

Merged

Conversation

thesujai
Copy link
Contributor

@thesujai thesujai commented Sep 15, 2023

Description

This change will not allow the input fields(Which has preprocessed input) of any page to appear focused when navigated with keyboard.
I have described the problem in this comment

Issue addressed

Fixes: #9077

Before/after screenshots

Before
Screencast from 15-09-23 04:49:34 PM IST.webm

After:

Screencast.from.15-09-23.04.46.38.PM.IST.webm

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

@MisRob MisRob self-requested a review September 18, 2023 12:16
@MisRob
Copy link
Member

MisRob commented Sep 22, 2023

Hi @thesujai, thanks for the contribution! I tested it a bit and looking at your comment here learningequality/kolibri#9077 (comment) and the code change, it makes much sense to me. I think your understanding of the problem is great. Thanks for debugging it thoroughly.

Looking at the history of KTextBox I can see that the line in the watcher your removed was in this component since the very beginning. KTextBox went through many other updates since then in regards to focus outline, so it is quite possible we can remove this safely. I think we will need to indeed do this change to fix the issue, but let me connect with our QA team next week to test for regressions in Kolibri just to be confident that it won't break anything else. It might take several days as we have quite full QA queue. I'll be in touch.

@MisRob
Copy link
Member

MisRob commented Sep 28, 2023

@thesujai Congratulations and thank you!

Our QA team just confirmed all is good. I will do a few last touches here in regards to our changelog and then merge.

@MisRob
Copy link
Member

MisRob commented Sep 28, 2023

@rtibbles I sneaked in renaming the changelog check action (#451) from check to check-changelog here because on this PR I noticed that check was shown in the settings of required checks which doesn't really say much. @thesujai apologies for my mess.

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.

Great work @thesujai

@MisRob MisRob merged commit f20bdad into learningequality:develop Sep 28, 2023
@thesujai
Copy link
Contributor Author

@MisRob Thank you!
I found this community active and helpful, I will look forward to contributing more.
If there are any good first issue with inactive assignee(As all are assigned) or a relatively simple issue please let me know.
Thank You:)

@thesujai thesujai deleted the two-input-focused-at-same-time branch September 29, 2023 07:47
@MisRob
Copy link
Member

MisRob commented Sep 29, 2023

Thanks @thesujai, I'm glad to hear that.

We're constantly adding new issues that are open for contribution so you're welcome to check Kolibri good first issues, Kolibri Design System ones, as well as Studio ones.

Just in case we run out of them again, and you'd like to work on something, contact us in GitHub Discussions of one of those projects. We have plenty of work, just sometimes not managing to open issues in time :) I just fed some more into Kolibri backlog.

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