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

Replace KResponseWindow with useKResponseWindow in Device plugin #11349

Merged

Conversation

thesujai
Copy link
Contributor

@thesujai thesujai commented Oct 4, 2023

Summary

I searched for all .vue in kolibri/plugins/device, and i started to replace KResponsiveWindow with useKResponsiveWindow

For testing:

  1. First Identified where ResponsiveWindow elements are are used
  2. Then play with CSS class of components that will be used when windowBreakpoint is reached.
  3. Then verify that the CSS tweaks is consistent with both KResponsiveWindow and userKResponsiveWindow
  4. Roll back the CSS tweaks made while testing

Page Affected

  • en/device/#/content
  • en/device/#/content/manage_tasks
  • en/device/#/content/manage_channel
  • en/device/#/content/channels
  • en/device/#/facilities
  • en/device/#/info

References

Fixes #11322

Reviewer guidance

Would it be possible to add a hacktoberfest-accepted label in this PR?
Here is the link: https://hacktoberfest.com/participation/#maintainers
Its fine if for some reasons you aren't able to do so:)


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: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) SIZE: very small labels Oct 4, 2023
@thesujai
Copy link
Contributor Author

thesujai commented Oct 4, 2023

@MisRob please have a look

@thesujai thesujai marked this pull request as ready for review October 5, 2023 14:30
@thesujai thesujai changed the title Replace KResponseWindow with userKResponseWindow Replace KResponseWindow with userKResponseWindow in Device plugin Oct 5, 2023
@MisRob MisRob added the TODO: needs review Waiting for review label Oct 6, 2023
@MisRob
Copy link
Member

MisRob commented Oct 6, 2023

@thesujai Thanks! I will review code in more detail later on, but briefly skimmed through it and looks great.

Can you recall which pages are affected? For our QA team, it'd be helpful to have a list of pages from the user-point of view, for example their URLs

@MisRob
Copy link
Member

MisRob commented Oct 6, 2023

^ This would also helps us in your other PRs

@MisRob
Copy link
Member

MisRob commented Oct 6, 2023

Here's a nice example #11346 (comment)

@thesujai
Copy link
Contributor Author

thesujai commented Oct 6, 2023

Yes sure

@thesujai
Copy link
Contributor Author

thesujai commented Oct 6, 2023

@MisRob For few changes which I did, I was not able to visually find the page in which it is reflected due to my less familiarity with the codeabse. Like file device/assets/src/views/ManageContentPage/ChannelPanel/ChannelDetails.vue and device/assets/src/views/SelectContentPage/index.vue.

@thesujai thesujai changed the title Replace KResponseWindow with userKResponseWindow in Device plugin Replace KResponseWindow with useKResponseWindow in Device plugin Oct 7, 2023
@MisRob
Copy link
Member

MisRob commented Oct 10, 2023

Thank you @thesujai, it's okay especially when you mentioned the places that you couldn't locate. I will try it out.

@rtibbles rtibbles added the hacktoberfest-accepted A label to apply to PRs that have been merged and can be used for participation in hacktoberfest label Oct 11, 2023
@MisRob
Copy link
Member

MisRob commented Oct 12, 2023

@thesujai Could you please rebase on top of the latest develop? That should hopefully resolve failing test check.

@thesujai
Copy link
Contributor Author

Hey @MisRob, can say a suitable method for rebasing it, last time i tried in another pr and it got messed up

@MisRob
Copy link
Member

MisRob commented Oct 13, 2023

@thesujai Here are the guidelines #11361.

However, let's wait, we figured out meanwhile that there's more things that need to be fixed so rebase wouldn't help at this point. I will review and eventually rebase this later on.

@thesujai
Copy link
Contributor Author

Okay @MisRob

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.

Code is fine, I tested all the places and haven't observed any regressions or console errors.

@MisRob
Copy link
Member

MisRob commented Oct 13, 2023

Let's hold merging though for some time. As soon as tests are fixed on develop, I will rebase this PR and merge it.

@MisRob MisRob merged commit 1bf619c into learningequality:develop Oct 26, 2023
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) hacktoberfest-accepted A label to apply to PRs that have been merged and can be used for participation in hacktoberfest SIZE: medium SIZE: small SIZE: very small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants