Skip to content
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

Jump to bottom

fix: open Drilldrown with the currently opened (sub)menu height #11416 #11425

Merged
merged 6 commits into from
Aug 4, 2018
Merged

fix: open Drilldrown with the currently opened (sub)menu height #11416 #11425

merged 6 commits into from
Aug 4, 2018

Conversation

ncoden
Copy link
Contributor

@ncoden ncoden commented Jul 30, 2018 •
edited
Loading

Description

When a Drilldown submenu is opened, then the whole Drilldown is closed and reopened, the submenu is still there but the Drilldown takes the height of the main menu.

This pull request fix this bug by using the active (sub)menu height when the Drilldown height is recalculated.

Changes

  • Save the currently opened sub-menu as $currentMenu
  • When calculating the Drilldown wrapper height, use the currently opened menu height instead of the primary menu.

⚠️ ToDo

  • Add unit tests

Types of changes

  • Documentation
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing
    functionality to change)

Checklist (all required):

  • I have read and follow the CONTRIBUTING document.
  • There are no other pull request similar to this one.
  • The pull request title is descriptive.
  • The template is fully and correctly filled.
  • The pull request targets the right branch (develop or develop-v...).
  • My commits are correctly titled and contain all relevant information.
  • My code follows the code style of this project.
  • I have updated the documentation accordingly to my changes (if relevant).
  • I have added tests to cover my changes (if relevant).
  • All new and existing tests passed.

@ncoden
fix: set the Drilldown height for on the currently opened (sub)menu f…
c7ce8d5 
…oundation#11416

Changes:
- Save the currently opened sub-menu as `$currentMenu`
- When calculating the Drilldown wrapper height, use the currently opened menu height instead of the primary menu.

Closes foundation#11416
@ncoden ncoden added this to the 6.5.0 milestone Jul 30, 2018
@ncoden
Copy link
Contributor Author

ncoden commented Jul 30, 2018

Note: I still need to add unit tests for this bug.

@ncoden ncoden mentioned this pull request Jul 30, 2018
Closed
4 tasks
ncoden added 4 commits July 31, 2018 23:54
@ncoden
test: add unit tests for Drilldown resizing on toggling
43c1fac
@ncoden
test: check the Drilldrown wrapper height instead of the element height
7f7dfd8 
Test the dynamic height of the wrapper instead of the static height of the element (data-drilldrown inside the wrapper).
@ncoden
fix: set a default for the current menu in Drilldown before it is used
90c4e45 
`$currentMenu` is used by `_getMaxDims`.

Changes: move `$currentMenu` initialization to `_init()`
@ncoden
test: add unit test for the Drilldown height when reopened from a sub…
1db6a1f 
…menu foundation#11416

See foundation#11416
@ncoden ncoden removed the WIP label Jul 31, 2018
@ncoden
Copy link
Contributor Author

ncoden commented Jul 31, 2018

I fixed a bug on initialization and added unit tests for the Drilldown resizing in the following cases (when toggled):

  • Opened
  • Opened then closed
  • Opened, shown a submenu, closed then reopened.

@ncoden ncoden requested review from DanielRuf and Owlbertz July 31, 2018 22:34
@Owlbertz
Copy link
Contributor

Owlbertz commented Aug 2, 2018

Will have a look asap.

@Owlbertz
Copy link
Contributor

Owlbertz commented Aug 2, 2018

Ok, I checked this but it kinda feels like it still has some issue...

What I did:

  1. Open http://localhost:3000/drilldown-menu.html
  2. Go to autoHeight version.
  3. Open Item 3 -> Item 3E
  4. Close menu using ESC
  5. Open Item 3
    -> Now level 3 is already visible (which I assume is a separate bug as it is already present in the docs)
    -> Despite level 3 is already visible, the menu only has the height of level 2 (everything below Item 3EE isn't visible).

Did I get something wrong or is there actually something odd with the menu?

@ncoden
Copy link
Contributor Author

ncoden commented Aug 2, 2018

@Owlbertz I tested it on develop and I have the same error. This is definitely something we should fix but it's not related to this pull request.

@Owlbertz
Copy link
Contributor

Owlbertz commented Aug 3, 2018

@ncoden while this is true, I am struggling reproducing the issue you fixed with the PR. Maybe you could give some more info on how to reproduce?

@ncoden
Copy link
Contributor Author

ncoden commented Aug 3, 2018

@Owlbertz See #11416

  1. Scale your browser down to see the toggle Button
  2. Open Mobile Menu, and click "Produkte OPEN ME"
  3. Click "Produkte 2 OPEN ME"
  4. Close and reopen Mobile Menu

Owlbertz
Owlbertz approved these changes Aug 4, 2018
View reviewed changes
Copy link
Contributor

@Owlbertz Owlbertz left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Owlbertz
Copy link
Contributor

Owlbertz commented Aug 4, 2018

Sorry, didn't get the referenced issue. With the example provided the issue becomes pretty obvious.

@ncoden
Copy link
Contributor Author

ncoden commented Aug 4, 2018

Thank you for the review @Owlbertz :)

I asked you a review because you worked on the Drilldrown before. Do you want me to poke you on future pull requests for others components ?

@ncoden ncoden merged commit 7959ef7 into foundation:develop Aug 4, 2018
ncoden added a commit to ncoden/foundation-sites that referenced this pull request Aug 25, 2018
@ncoden
Use pull request foundation#11425 from ncoden/fix/drilldown-submenu-h…
6ed3621 
…eight-11416 for v6.5.0

c7ce8d5 fix: set the Drilldown height for on the currently opened (sub)menu foundation#11416
43c1fac test: add unit tests for Drilldown resizing on toggling
7f7dfd8 test: check the Drilldrown wrapper height instead of the element height
90c4e45 fix: set a default for the current menu in Drilldown before it is used
1db6a1f test: add unit test for the Drilldown height when reopened from a submenu foundation#11416
d4617ac clean: remove infinite timeout used for development in Drilldown tests

Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
@ncoden ncoden mentioned this pull request Sep 10, 2018
Merged
10 tasks
This was referenced Apr 22, 2020
Open
Open
Merged
This was referenced Apr 22, 2020
Open
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@Owlbertz Owlbertz Owlbertz approved these changes

@DanielRuf DanielRuf Awaiting requested review from DanielRuf

Assignees
No one assigned
Labels
bug fix Drilldown Menu
Projects
None yet
Milestone
6.5.0
Development

Successfully merging this pull request may close these issues.

F6.5 Drilldown Submenu height calculation wrong when toggled
2 participants
@ncoden @Owlbertz