feat(Dropdown): open to side with most space left #720 #2817

mihai-dinculescu · 2018-05-21T15:22:07Z

I have added a new prop, "keepInViewPort", so dropdown is consistent to Popup. However, this one is false by default, so the current behavior doesn't change.

Please let me know your thoughts.

codecov-io · 2018-05-21T15:27:36Z

Codecov Report

Merging #2817 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2817      +/-   ##
==========================================
+ Coverage   99.66%   99.74%   +0.07%     
==========================================
  Files         161      160       -1     
  Lines        2722     2785      +63     
==========================================
+ Hits         2713     2778      +65     
+ Misses          9        7       -2

Impacted Files	Coverage Δ
src/modules/Dropdown/Dropdown.js	`100% <100%> (ø)`	⬆️
src/modules/Search/Search.js	`99.46% <0%> (-0.01%)`	⬇️
src/modules/Tab/TabPane.js	`100% <0%> (ø)`	⬆️
src/elements/Segment/Segment.js	`100% <0%> (ø)`	⬆️
src/elements/Header/HeaderSubheader.js	`100% <0%> (ø)`	⬆️
src/elements/Button/ButtonOr.js	`100% <0%> (ø)`	⬆️
src/addons/Pagination/Pagination.js	`100% <0%> (ø)`	⬆️
src/views/Card/CardHeader.js	`100% <0%> (ø)`	⬆️
src/elements/Image/ImageGroup.js	`100% <0%> (ø)`	⬆️
src/elements/Label/LabelGroup.js	`100% <0%> (ø)`	⬆️
... and 146 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecd238e...1e45dfd. Read the comment docs.

levithomason · 2018-05-21T18:27:30Z

Let's make this the default behavior and remove the prop. The current behavior is a bug so it is OK to "break" it.

levithomason · 2018-05-21T18:29:21Z

src/modules/Dropdown/Dropdown.js

+    } else {
+      this.setState({ shouldOpenUpward: false })
+    }
+  }


An if/else which results in true/false statements can be simplified to using the condition as the value:

this.setState({ shouldOpenUpward: spaceAtTheBottom < 0 && spaceAtTheTop > 0, })

In the case that both top and bottom are blocked, perhaps we should open to the side with the most available space. I think this math is wrong, but something like:

this.setState({ shouldOpenUpward: spaceAtTheBottom > spaceAtTheTop })

levithomason · 2018-05-21T18:34:46Z

src/modules/Dropdown/Dropdown.js

+    if (!menu) return
+
+    const dropdownRect = this.ref.getBoundingClientRect()
+    const itemsHeight = menu.clientHeight


nit: Let's keep the naming consistent and use menuHeight.

levithomason · 2018-05-21T18:39:10Z

src/modules/Dropdown/Dropdown.js

@@ -1273,7 +1338,7 @@ export default class Dropdown extends Component {
      useKeyOnly(selection, 'selection'),
      useKeyOnly(simple, 'simple'),
      useKeyOnly(scrolling, 'scrolling'),
-      useKeyOnly(upward, 'upward'),
+      useKeyOnly(upward || shouldOpenUpward, 'upward'),


When we have props that need to be controlled, we move to them to being autoControlledProps. See Dropdown.js around line 346.

add upward to static autoControlledProps = []

add defaultUpward as a propType (since it is now stateful, we accept a default value for it)

move all usages of upward from this.props.upward to this.state.upward

to change the value of upward use this.trySetState({ upward: <new value> })

remove shouldOpenUpward as it is no longer needed.

Auto controlled props automatically defer to the user's prop value, if any. Also, this.trySetState() will not update the state value if the user is using that prop.

levithomason · 2018-05-21T18:40:00Z

Looking great! I've added some feedback for you here. Thanks much for tackling this one.

mihai-dinculescu · 2018-05-22T15:25:36Z

Thank you for the feedback. I've done the changes that you've requested.

However, there is one unit test failing now: "filters the items based on custom search function"

It fails because the search function is now called two times instead of once. I suspect that it's called the first time when the dropdown opens, and a second time after the direction is set.

This also points another issue. I don't like the fact that the dropdown needs to rerender after the direction is set. I'm not sure how to get around this though. The content needs to be rendered before we can calculate it's height.

I could use some help.

samboylett · 2018-05-24T09:01:40Z

I've been looking at this too. I think if it doesn't change direction, it should only render once, so the search function will only be called once. Otherwise it will have to render twice (there's not really any way around this) so the search function will be called twice too.

mihai-dinculescu · 2018-05-24T11:40:53Z

Thank you for your input, @samboylett.

I've optimized the code so it updates the state only when there is a relevant change.

levithomason · 2018-05-31T22:06:57Z

This looks great, thanks! The last thing we need here is a test. The test would resize the browser window and assert the dropdown opened upward. We can merge after this update.

levithomason · 2018-06-01T17:25:10Z

Superb 👍 I've merged master. We can merge this PR once tests pass.

levithomason · 2018-06-01T17:33:58Z

@mihai-dinculescu You've contributed here before and you've shown care and initiative for ensuring SUIR becomes better. We appreciate this and want to enable you to do more. I've added you as a collaborator. You're now able to manage issues and PRs as well as create branches here in the main repo. You no longer need to work from a fork. Merging to master is the only reserved permission.

We are very select about who we add and when, so know that we trust your abilities and your judgment. Please feel free to weigh in on any issues and PRs that you have an opinion about. You have a share of ownership and say here now.

Once you accept the invite (see your email), you might want to show the Semantic Org badge publicly on your profile. To do that, navigate to the "people" page and change the "private" dropdown to "public" for your user in the table. Here's a direct link to your user in the table.

/cc
@Semantic-Org/react-collaborators
@Semantic-Org/react-maintainers

Please welcome @mihai-dinculescu to our ranks 😄 🎉

mihai-dinculescu · 2018-06-01T19:06:45Z

Thank you @levithomason. I'll do my best to be helpful.

levithomason · 2018-06-03T03:14:55Z

Released in [email protected].

Feature implementation.

1e5c8fa

Mihai Dinculescu added 3 commits May 21, 2018 18:19

Docs fix.

39a14d5

PropTypes update.

a4cd957

Basic tests.

c6e9dc5

levithomason reviewed May 21, 2018

View reviewed changes

levithomason added the needs author feedback label May 21, 2018

Code review fixes.

271bb01

Optimize state update.

3265024

mihai-dinculescu force-pushed the dropdown_keep_in_viewport branch from bf7475e to 3265024 Compare May 24, 2018 10:59

Add upward tests.

c84c9c8

levithomason added ready for merge and removed needs author feedback labels Jun 1, 2018

Merge branch 'master' into dropdown_keep_in_viewport

1e45dfd

levithomason merged commit 5ebeedf into Semantic-Org:master Jun 1, 2018

Menci mentioned this pull request Aug 27, 2020

Dropdown won't open to *horizontal* side with most space left #4048

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Dropdown): open to side with most space left #720 #2817

feat(Dropdown): open to side with most space left #720 #2817

mihai-dinculescu commented May 21, 2018 •

edited

Loading

codecov-io commented May 21, 2018 •

edited

Loading

levithomason commented May 21, 2018

levithomason May 21, 2018

levithomason May 21, 2018

levithomason May 21, 2018

levithomason May 21, 2018

levithomason commented May 21, 2018

mihai-dinculescu commented May 22, 2018 •

edited

Loading

samboylett commented May 24, 2018

mihai-dinculescu commented May 24, 2018

levithomason commented May 31, 2018

levithomason commented Jun 1, 2018

levithomason commented Jun 1, 2018 •

edited

Loading

mihai-dinculescu commented Jun 1, 2018

levithomason commented Jun 3, 2018

feat(Dropdown): open to side with most space left #720 #2817

feat(Dropdown): open to side with most space left #720 #2817

Conversation

mihai-dinculescu commented May 21, 2018 • edited Loading

codecov-io commented May 21, 2018 • edited Loading

Codecov Report

levithomason commented May 21, 2018

levithomason May 21, 2018

Choose a reason for hiding this comment

levithomason May 21, 2018

Choose a reason for hiding this comment

levithomason May 21, 2018

Choose a reason for hiding this comment

levithomason May 21, 2018

Choose a reason for hiding this comment

levithomason commented May 21, 2018

mihai-dinculescu commented May 22, 2018 • edited Loading

samboylett commented May 24, 2018

mihai-dinculescu commented May 24, 2018

levithomason commented May 31, 2018

levithomason commented Jun 1, 2018

levithomason commented Jun 1, 2018 • edited Loading

mihai-dinculescu commented Jun 1, 2018

levithomason commented Jun 3, 2018

mihai-dinculescu commented May 21, 2018 •

edited

Loading

codecov-io commented May 21, 2018 •

edited

Loading

mihai-dinculescu commented May 22, 2018 •

edited

Loading

levithomason commented Jun 1, 2018 •

edited

Loading