-
Notifications
You must be signed in to change notification settings - Fork 751
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Replaced KResponsiveWindow mixin by useKResponsiveWindow composable - … #11365
Replaced KResponsiveWindow mixin by useKResponsiveWindow composable - … #11365
Conversation
…Media Player plugin
Build Artifacts
|
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 property missed!
mixins: [commonCoreStrings, responsiveWindowMixin, responsiveElementMixin], | ||
mixins: [commonCoreStrings, responsiveElementMixin], | ||
setup() { | ||
const { 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.
The windowIsPortrait
property is also used, so must be destructured here and returned from the setup method.
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.
I have made the necessary changes; please check it once.
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.
This doesn't cause any errors in manual testing, and is the change I expect.
… Media Player plugin (learningequality#11365) Replace KResponsiveWindow mixin by useKResponsiveWindow composable - Media Player plugin
… Media Player plugin (learningequality#11365) Replace KResponsiveWindow mixin by useKResponsiveWindow composable - Media Player plugin
…Media Player plugin
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)