-
Notifications
You must be signed in to change notification settings - Fork 712
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
AnMi: Replaced KResponsiveWindow mixin by useKResponsiveWindow compos… #11346
AnMi: Replaced KResponsiveWindow mixin by useKResponsiveWindow compos… #11346
Conversation
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.
A tweak to use the composition API fully here!
@@ -249,7 +249,8 @@ | |||
return this.$route.name === PageNames.DATA_EXPORT_PAGE; | |||
}, | |||
windowSizeStyle() { | |||
if (this.windowIsMedium || this.windowIsSmall) { | |||
const { windowIsMedium, windowIsSmall } = useKResponsiveWindow(); |
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.
It is best to consume the composable via the composition API, so instead of making the modification here, you would add a setup()
method to the component, like this:
setup() {
const { windowIsMedium, windowIsSmall } = useKResponsiveWindow();
return { windowIsMedium, windowIsSmall };
},
this.windowIsMedium
and this.windowIsSmall
will now be defined on the component, as they are returned as properties of the setup function return value.
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 @rtibbles thanks for the hint. I tried to fix it as you suggested.
@andreamisuraca Thank you for the contribution! Can you recall which pages you tested manually? There's no need to be uploading screenshots or go into much detail, just jotting down briefly what places in the app are affected (e.g. URL or brief steps to navigate to a page) would help the QA team to test for regressions. |
Yes, please have a look at a note about imports here #11321 |
Build Artifacts
|
@MisRob thank you for the hint. I checked the following pages and forms:
|
b108426
to
046e8dc
Compare
…able in Facility plugin
046e8dc
to
d1a305a
Compare
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.
One place where it looks like the wrong composable attribute has been used. Seems our linting is not catching this just yet!
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.
No observed regressions during manual testing.
Thank you for your contribution, @andreamisuraca - much appreciated! Your responsiveness to feedback is commendable. |
… Facility plugin (learningequality#11346) * AnMi: Replaced KResponsiveWindow mixin by useKResponsiveWindow composable in Facility plugin * AnMi: Added setup method to the components * AnMi: Fixed unit test by adding missing mock * AnMi: Replaced wrong composable
… Facility plugin (learningequality#11346) * AnMi: Replaced KResponsiveWindow mixin by useKResponsiveWindow composable in Facility plugin * AnMi: Added setup method to the components * AnMi: Fixed unit test by adding missing mock * AnMi: Replaced wrong composable
… Facility plugin (learningequality#11346) * AnMi: Replaced KResponsiveWindow mixin by useKResponsiveWindow composable in Facility plugin * AnMi: Added setup method to the components * AnMi: Fixed unit test by adding missing mock * AnMi: Replaced wrong composable
Summary
Into the Facility plugin I replaced KResponsiveWindow mixin with useKResponsiveWindow composable.
References
#11323
Reviewer guidance
Just a replacement. Still some doubts about responsiveWindowMixin (without K), should it be changed too?
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)