Override constrainToScroll prop default value to false #586

kafukoM · 2024-03-21T07:07:33Z

Description

Options dropdown will open downwards as expected.

Issue addressed

Options dropdown opens upwards regardless of available space below.

Addresses #432

Before/after screenshots

BEFORE

AFTER

Changelog

#586
- Description: Adjust KDropdownMenu to open downwards when there's sufficient space, improving user experience and interface consistency.
- Products impact: Bugfix
- Addresses: #432
- Components: KDropdownMenu.vue
- Breaking: no
- Impacts a11y: no
- Guidance: This update ensures the KDropdownMenu component behaves more predictably in various layouts, enhancing UI consistency across applications. No action is needed from consumers aside from updating to the latest version to benefit from this improvement.

Steps to test

Go to Facility
Click Settings
Scroll down and create PIN if non-existent
Click Options button

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical and brittle code paths are covered by unit tests
The change is described in the changelog section above

After review

The changelog item has been pasted to the CHANGELOG.md

kafukoM · 2024-03-21T07:12:58Z

@MisRob Hello. Kindly requesting for your review.

akolson

Hi @kafukoM! Generally, changes look good to me. Just a minor change and we should be good. Thanks

akolson · 2024-03-21T11:24:08Z

lib/KDropdownMenu.vue

+       // Override default prop value
+      constrainToScrollParent: {
+        type: Boolean,
+        default: false


For consistency, I think this needs to default to true as is with Keen UI. We can then override the default if we need to by passing false. This will also ensure that we are backward compatible too.

There are also, 1 or 2 linting issues to take care of. You can fix any linting issues by running yarn run lint-fix before adding and committing your changes

For changelog build failures, please copy the changelog below(I made a few changes to your changelog from the pr) and add it to the CHANGELOG.md just below the ## Version 3.x.x (release-v3 branch) section as the latest entry, since your fix is targeting release-v3

- [#586] - **Description:** Adds a new prop `constrainToScrollParent ` to `KDropdownMenu` to allow overriding of its popover flipping behavior. - **Products impact:** Bugfix - **Addresses:** [#432](https://github.com/learningequality/kolibri-design-system/issues/432) - **Components:** KDropdownMenu - **Breaking:** no - **Impacts a11y:** no - **Guidance:** Use the `constrainToScrollParent` prop to override the default popover flipping behavior of the `KDropdownMenu` component prop where necessary. [#586]: https://github.com/learningequality/kolibri-design-system/pull/586

Please be sure to let me know in case you need any support in the process. Thanks again

akolson · 2024-03-21T12:57:21Z

lib/KDropdownMenu.vue

@@ -34,6 +35,11 @@
      UiMenu,
    },
    props: {
+       // Override default prop value


Can we replace this comment with documentation instead

/** * The dropdown menu popover flips its position to avoid overflows within the parent. Setting it to false disables the flipping behavior. */

@MisRob, happy to hear your thoughts if this developer friendly language :)

That'd be helpful documentation definitely, thank you

Thank you for the feedback @akolson. I have made the requested changes in the latest commit.

Does this mean I have to submit another PR showcasing the override for the template in Kolibiri rendering the KDropdownMenu component?

Yes, I think we could start by overriding the value in Kolibri: Facility Settings, where the bug was initially caught. Please see #432 for specifics. You can test your KDS changes locally in kolibri by running

yarn run devserver-with-kds /path/to/your/local/kolibri-design-system

The rest of the overrides will be done as and when its needed.

MisRob · 2024-03-22T05:34:20Z

yarn.lock

I don't think we want to be changing yarn.lock as part of this task. Could you please revert?

@MisRob I have reverted the commit, leaving out the yarn.lock changes

@kafukoM I think this change on yarn.lock is still present

@akolson @MisRob The new commit should have reverted the changes on the yarn.lock file

Thanks @kafukoM, although It looks like we picked up some conflicts in the process.

It looks like you are not having some changelog entries. All the below should be present in your version of the changelog

- [#586] - **Description:** Adds a new prop `constrainToScrollParent ` to `KDropdownMenu` to allow overriding of its popover flipping behavior. - **Products impact:** Bugfix - **Addresses:** [#432](https://github.com/learningequality/kolibri-design-system/issues/432) - **Components:** KDropdownMenu - **Breaking:** no - **Impacts a11y:** no - **Guidance:** Use the `constrainToScrollParent` prop to override the default popover flipping behavior of the `KDropdownMenu` component prop where necessary. [#586]: https://github.com/learningequality/kolibri-design-system/pull/586 - [#552] - **Description:** New `KListWithOverflow` component. - **Products impact:** new API. - **Addresses:** https://github.com/learningequality/kolibri-design-system/issues/556, https://github.com/learningequality/studio/issues/3423, https://github.com/learningequality/kolibri/issues/11923. - **Components:** KListWithOverflow. - **Breaking:** no. - **Impacts a11y:** no. - **Guidance:** -. [#552]: https://github.com/learningequality/kolibri-design-system/pull/552 - [#552] - **Description:** New `useKResponsiveElement` private composable, `KResponsiveElementMixin` translated to this composable. - **Products impact:** -. - **Addresses:** -. - **Components:** -. - **Breaking:** no. - **Impacts a11y:** no. - **Guidance:** -. [#552]: https://github.com/learningequality/kolibri-design-system/pull/552 - [#538] - **Description:** Complete KImg implementation - **Products impact:** new API - **Addresses:** https://github.com/learningequality/kolibri-design-system/issues/368 - **Components:** KImg - **Breaking:** no - **Impacts a11y:** yes - **Guidance:** One of the benefits of using KImg is that it throws a11y related warnings [#538]: https://github.com/learningequality/kolibri-design-system/pull/538

@akolson I have updated my changelog file with the latest commit

@kafukoM, the above was only intended as a guide for you when resolving the conflict locally, my apologies for any confusion. Could you please revert your most recent commit and do the following?

git rebase release-v3 or git merge release-v3. This will bring in any changes you don't have

Resolve the conflicts,

commit the changes

Finally, push

Another more straightforward option would be to undo all changes made in the changelog, then rebase or merge release-v3 into your branch after which you can add the changelog here again.

@akolson I think my latest commit should have resolved the conflict

Thanks @kafukoM. please see my other comment here

This reverts commit 44409f6.

rather than applying them to <img> in preparation for implementing the image scaling logic.

akolson · 2024-03-22T09:59:07Z

🙂

akolson · 2024-03-22T10:00:48Z

@MisRob I am happy for this to be merged, if everything is okay on your side :)

akolson

Looks good to me! Great job @kafukoM!

MisRob · 2024-03-22T12:44:36Z

@MisRob I am happy for this to be merged, if everything is okay on your side :)

Sure @akolson, you know more than me about this issue :) Thank you.

akolson · 2024-03-22T12:48:56Z

I'll proceed with the merge.

MisRob · 2024-03-22T12:49:55Z

Okay. Do we need Kolibri follow up issue for this to take effect to be able close this #432 or will the new default value resolve it automatically?

akolson · 2024-03-22T12:52:11Z

We need a follow up pr. @kafukoM has already opened one here. Once we have the KDS NPM package ready, we should be good for the merge for it.

MisRob · 2024-03-22T13:02:23Z

Ah I see, perfect

Override constrainToScroll prop default value to false

5d0e433

MisRob requested a review from akolson March 21, 2024 07:33

MisRob added the TODO: needs review Waiting for review label Mar 21, 2024

akolson requested changes Mar 21, 2024

View reviewed changes

akolson reviewed Mar 21, 2024

View reviewed changes

Updated default constrainToScrollParent prop value

f1b8ee0

MisRob reviewed Mar 22, 2024

View reviewed changes

kafukoM and others added 22 commits March 22, 2024 09:17

Reverted yarn.lock changes

837d6e2

Modified changelog.md

44409f6

Revert "Modified changelog.md"

ec8cf3b

This reverts commit 44409f6.

Updated CHANGELOG.md

996d695

Move dimension styles to the container

b577fa8

rather than applying them to <img> in preparation for implementing the image scaling logic.

Add scaling and ratio logic

f9fcaf0

Document default slot

8ea6972

Add slots and placeholder logic

e358e4b

Add border radius

3d3107a

Cleanup documentation and tests

1b51e44

Trying with hidden overflow flex wrapper

86a55a3

Overflow items with visibility hidden items

3f55c9e

Ensuring all visible items if moreButton were not rendered

98018f4

Polishing example

bcf6fa2

Add docs page

ce80b6a

Add divider support

4b20918

Add appearance overrides

add2f37

Add useKResponsiveElement

5f9fd92

Watch responsive elementWidth in KListWithOverflow

4ad6e9d

Update docs

4c0850d

Add component documentation

e3cfdb3

Add changelog

691b74a

AlexVelezLl and others added 8 commits March 22, 2024 12:19

Lint files

f3690dd

Rename composable to private

e24ea56

Set list as overflow hidden

5f25479

Replace ResizeSensor

b4d4ea2

Requested changes

f8b5ef8

Increase version

76c72de

Resolved CHANGELOG.md conflicts

8dbb67b

Merge branch 'release-v3' into kdropdown-options-update

883ab3d

kafukoM mentioned this pull request Mar 22, 2024

Overrides prop default flipping behaviour learningequality/kolibri#12015

Merged

9 tasks

akolson requested review from akolson and MisRob March 22, 2024 12:42

akolson approved these changes Mar 22, 2024

View reviewed changes

akolson merged commit 045fa85 into learningequality:release-v3 Mar 22, 2024
8 checks passed

MisRob mentioned this pull request May 11, 2024

KDropdownMenu: unexpected behavior in "Facility Settings" view #432

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Override constrainToScroll prop default value to false #586

Override constrainToScroll prop default value to false #586

kafukoM commented Mar 21, 2024 •

edited

Loading

kafukoM commented Mar 21, 2024

akolson left a comment

akolson Mar 21, 2024

akolson Mar 21, 2024

akolson Mar 21, 2024

akolson Mar 21, 2024

akolson Mar 21, 2024 •

edited

Loading

MisRob Mar 22, 2024

kafukoM Mar 22, 2024

kafukoM Mar 22, 2024

akolson Mar 22, 2024

MisRob Mar 22, 2024

kafukoM Mar 22, 2024

akolson Mar 22, 2024

kafukoM Mar 22, 2024

akolson Mar 22, 2024

akolson Mar 22, 2024

kafukoM Mar 22, 2024

akolson Mar 22, 2024 •

edited

Loading

kafukoM Mar 22, 2024

akolson Mar 22, 2024

akolson commented Mar 22, 2024

akolson commented Mar 22, 2024 •

edited

Loading

akolson left a comment

MisRob commented Mar 22, 2024

akolson commented Mar 22, 2024

MisRob commented Mar 22, 2024

akolson commented Mar 22, 2024

MisRob commented Mar 22, 2024

Override constrainToScroll prop default value to false #586

Override constrainToScroll prop default value to false #586

Conversation

kafukoM commented Mar 21, 2024 • edited Loading

Description

Issue addressed

Before/after screenshots

Changelog

Steps to test

Testing checklist

After review

kafukoM commented Mar 21, 2024

akolson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akolson Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akolson Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akolson commented Mar 22, 2024

akolson commented Mar 22, 2024 • edited Loading

akolson left a comment

Choose a reason for hiding this comment

MisRob commented Mar 22, 2024

akolson commented Mar 22, 2024

MisRob commented Mar 22, 2024

akolson commented Mar 22, 2024

MisRob commented Mar 22, 2024

kafukoM commented Mar 21, 2024 •

edited

Loading

akolson Mar 21, 2024 •

edited

Loading

akolson Mar 22, 2024 •

edited

Loading

akolson commented Mar 22, 2024 •

edited

Loading