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 KResponsiveWindow mixin by useKResponsiveWindow composable #11474

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

KshitijThareja
Copy link
Contributor

Summary

This PR replaces KResponsiveWindow with useKResponsiveWindow in Setup Wizard plugin.

References

This PR aims to resolve issue #11330

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: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) SIZE: small labels Oct 29, 2023
@KshitijThareja KshitijThareja marked this pull request as draft October 30, 2023 04:33
@KshitijThareja KshitijThareja marked this pull request as ready for review October 30, 2023 04:33
@KshitijThareja
Copy link
Contributor Author

@nucleogenesis @marcellamaki Is the updated code fine or are there any changes required in this PR?

@MisRob
Copy link
Member

MisRob commented Nov 10, 2023

Hi @KshitijThareja, thank you for your contribution! We will definitely follow-up, there's been lots of pull requests in our queue.

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

Hi @KshitijThareja - thanks so much for working on this PR. You're off to a good start. The removal of the responsiveWindowMixin and using the composable in the setup is right. However, you're not quite there yet.

  1. You need to make sure you are importing the composable in both places
  2. You need to make sure that your consts that you are using in the setup match what is actually needed by the file. Take a look at the options in the composable (you can refer to the original issue for more details about the design system if you need it) and what is needed in the file. Then make sure they align. (For example, you have imported windowIsPortrait in both files. Is that used anywhere? Don't import things that are not used, and do import everything that is needed).

Let me know if you have further questions, and thanks again for your work on this.

@KshitijThareja
Copy link
Contributor Author

@marcellamaki thanks for your review. I understand I made some mistakes in the code. I've corrected everything and pushed the changes.
Again, thanks for pointing it out :)

@KshitijThareja
Copy link
Contributor Author

@marcellamaki Is there any specific reason for the DMG build to fail?

@marcellamaki marcellamaki self-requested a review November 13, 2023 15:20
Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

Hi @KshitijThareja - we recently updated the DMG github action on our end, so I re-ran the job and that resolved it. Thank you also for the updates you made! Looks good to me. We appreciate your contribution to Kolibri 🎉

@marcellamaki marcellamaki merged commit 0fef16d into learningequality:develop Nov 13, 2023
@KshitijThareja
Copy link
Contributor Author

Thanks 😄 @marcellamaki

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) SIZE: small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants