-
Notifications
You must be signed in to change notification settings - Fork 745
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
Conversation
Build Artifacts
|
@nucleogenesis @marcellamaki Is the updated code fine or are there any changes required in this PR? |
Hi @KshitijThareja, thank you for your contribution! We will definitely follow-up, there's been lots of pull requests in our queue. |
There was a problem hiding this 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.
- You need to make sure you are importing the composable in both places
- 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 importedwindowIsPortrait
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.
@marcellamaki thanks for your review. I understand I made some mistakes in the code. I've corrected everything and pushed the changes. |
@marcellamaki Is there any specific reason for the DMG build to fail? |
There was a problem hiding this 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 🎉
Thanks 😄 @marcellamaki |
Summary
This PR replaces
KResponsiveWindow
withuseKResponsiveWindow
in Setup Wizard plugin.References
This PR aims to resolve issue #11330
Reviewer guidance
…
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)