feat: add possibility nested dropdown #290

romainlancia · 2021-04-06T11:52:23Z

Component:
Dropdown
Description:
Add possibility to put a Dropdown as an item of Dropdown
Design:
https://user-images.githubusercontent.com/47044686/115277099-c37fb100-a13b-11eb-80bc-bb2687e69221.mp4

Closes: #277

bert-e · 2021-04-06T11:52:24Z

Hello lebroz,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

bert-e · 2021-04-06T11:52:27Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

JBWatenbergScality

Few comments below that we may want to address before merging this.
Another important fact I noticed is that the dropdown is not navigable through keyboard interactions (tab or directional arrows). It is not specific to this feature but this dropdown in dropdown implementation makes this an even farer goal to achieve, maybe it is something we should keep in mind as well when working on such features.

JBWatenbergScality · 2021-04-06T12:40:48Z

src/lib/components/dropdown/Dropdown.component.js


 export type Item = {
  label: string,
  name?: string,
  selected?: boolean,
-  onClick: any => void
+  onClick?: any => void,


You might want to type this to SyntheticMouseEvent<HTMLDivElement> (ref : https://flow.org/en/docs/react/events/)

JBWatenbergScality · 2021-04-06T12:41:05Z

src/lib/components/dropdown/Dropdown.component.js

+  onBlur: any => void,
+  onFocus: any => void,
+  onClick: any => void,
+  onMouseEnter: any => void,
+  onMouseLeave: any => void,


You might want to type this to SyntheticMouseEvent (ref : https://flow.org/en/docs/react/events/)

JBWatenbergScality · 2021-04-06T12:41:18Z

src/lib/components/dropdown/Dropdown.component.js

-  caret?: boolean
+  caret?: boolean,
+  dataIndex?: number,
+  onClick?: any => void,


You might want to type this to SyntheticMouseEvent (ref : https://flow.org/en/docs/react/events/)

JBWatenbergScality · 2021-04-06T13:04:53Z

src/lib/components/dropdown/utils.js

+  else {
+    // Check collision for dropdown acting as a root button*
+  if (isBottomHit) {


nit: Please format this using prettier

JBWatenbergScality · 2021-04-06T13:25:50Z

src/lib/components/dropdown/utils.js

+  }
+}
+
+export const getPositionDropdownMenu = ({ isItem, triggerSize, size, nbItems, itemIndex }) => {


This method should be unit tested

JBWatenbergScality · 2021-04-06T13:27:20Z

src/lib/components/dropdown/utils.js

+    if (isRightHit && isBottomHit) {
+      return css`
+          right: ${size.width}px;
+          bottom: ${-triggerSize.height * (itemIndex - nbItems + 1)}px;


What if some items takes more than one line ?

@JBWatenbergScality Trying to find a solution but this one is a bit tricky to handle because indeed if an item have a height superior to other item, the new menu generated will display with a wrong offset. The problem I have to get each item size of the previous menu to place the new menu correctly but it will add a lot of complexity to the code. I can figure out a way to implement that in a simple way, do you have any suggestions ?

JBWatenbergScality · 2021-04-06T14:50:52Z

src/lib/components/dropdown/Dropdown.component.js

 function Dropdown({
+  isItem = false,


I think separating exposed Dropdown component and an internal non-exposed DropdownAsAnItem component would be a better design. It would for example avoid confusion of having an isItem or dataIndex(which are implementation details) props available when trying to use the Dropdown component. I think it is very important to keep our exposed API very clear and not add props linked to the implementation in what we expose.

bert-e · 2021-04-06T14:54:59Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

The following reviewers are expecting changes from the author, or must review again:

@JBWatenbergScality

ChengYanJin · 2021-04-06T15:04:27Z

src/lib/components/dropdown/Dropdown.component.js

@@ -41,43 +63,19 @@ const DropdownMenuStyled = styled.ul`
  position: absolute;
  margin: 0;
  padding: 0;
-  border: 1px solid ${getThemePropSelector("primary")};


Please double-check with the design, we should have a border for each of the items.

ChengYanJin · 2021-04-06T15:07:45Z

src/lib/components/dropdown/Dropdown.component.js

+        open={open}
+        isItem={isItem}
+        onMouseEnter={(e) => {
+          setItemIndex(e && e.target && e.target.getAttribute('data-index') || 0)


We got a flow error. https://eslint.org/docs/rules/no-mixed-operators
setItemIndex(e && e.target && e.target.getAttribute('data-index')) || 0

ChengYanJin · 2021-04-06T15:10:40Z

src/lib/components/dropdown/Dropdown.component.js

@@ -101,86 +99,125 @@ const Caret = styled.span`

 const TriggerStyled = ButtonStyled.withComponent("div");

+const DropdownTriggerContainer = forwardRef<DropdownTriggerContainerProps, Element>(({isItem, dataIndex, open, size , variant, title, onBlur, onFocus, onClick, onMouseEnter, onMouseLeave, children, ...rest}, ref) => {


I am wondering do you have the prettier extension installed? Please enable it to make sure the code follows the .prettierrc config.
Thanks!

ChengYanJin · 2021-04-06T15:29:48Z

src/lib/components/dropdown/Dropdown.component.js

-  onClick: any => void
+  onClick?: any => void,
+  submenuIcon?: Node,
+  submenuItems?: Array<Item>


As shown in the design, the documentation and support item also have another external link icon.
Maybe we miss another iconExternal prop?

ChengYanJin · 2021-04-06T15:38:14Z

src/lib/components/dropdown/Dropdown.component.js

          <Caret>
-            <i className="fas fa-caret-down" />
+            <i className={`fas fa-caret-${isItem ? 'right' : 'down'}`} />


The right arrow icon should be chevron instead of caret
<i className="fas fa-chevron-right" />

bert-e · 2021-04-06T15:46:12Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

The following reviewers are expecting changes from the author, or must review again:

bert-e · 2021-04-08T15:43:31Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

The following reviewers are expecting changes from the author, or must review again:

romainlancia · 2021-04-08T17:02:20Z

@JBWatenbergScality @ChengYanJin Regarding prettier, I think it is best to format the code before someone pushes his work. I just launch npx prettier --check ./src and a lot of files are not following the prettier config.
For now, I just run prettier on my files in order to not push files that are not related to the scope of the ticket.
But, in a future ticket, we can maybe do something like this:

  "husky": {
    "hooks": {
      "pre-commit": "npx prettier --write ./src && npm run lint && npm run flow && git-diff --exit-code"
    }
  },

What do you think ?

…jest + simplify dropdown and fix menu position

romainlancia · 2021-04-19T18:03:21Z

here's a video of a demo: https://user-images.githubusercontent.com/47044686/115277099-c37fb100-a13b-11eb-80bc-bb2687e69221.mp4

feat: add possibility nested dropdown

70e53fb

romainlancia requested a review from alexis-ld April 6, 2021 11:52

Dropdown: udpate tests

de214c8

romainlancia requested review from MonPote and removed request for alexis-ld April 6, 2021 12:55

JBWatenbergScality requested changes Apr 6, 2021

View reviewed changes

ChengYanJin requested changes Apr 6, 2021

View reviewed changes

resolve comments

abc113b

romainlancia force-pushed the feature/dropdown-in-a-dropdown branch from 24480e9 to abc113b Compare April 8, 2021 16:45

romainlancia requested review from ChengYanJin and JBWatenbergScality April 8, 2021 17:08

add plugins to babel for injecting optional and coalescing syntax in …

e091c54

…jest + simplify dropdown and fix menu position

MonPote closed this Jan 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add possibility nested dropdown #290

feat: add possibility nested dropdown #290

romainlancia commented Apr 6, 2021 •

edited

Loading

bert-e commented Apr 6, 2021

bert-e commented Apr 6, 2021

JBWatenbergScality left a comment •

edited

Loading

JBWatenbergScality Apr 6, 2021

romainlancia Apr 8, 2021

JBWatenbergScality Apr 6, 2021

romainlancia Apr 8, 2021

JBWatenbergScality Apr 6, 2021

romainlancia Apr 8, 2021

JBWatenbergScality Apr 6, 2021

romainlancia Apr 8, 2021

JBWatenbergScality Apr 6, 2021

romainlancia Apr 8, 2021

JBWatenbergScality Apr 6, 2021

romainlancia Apr 8, 2021 •

edited

Loading

JBWatenbergScality Apr 6, 2021

romainlancia Apr 8, 2021

bert-e commented Apr 6, 2021

ChengYanJin Apr 6, 2021

romainlancia Apr 8, 2021

ChengYanJin Apr 6, 2021

romainlancia Apr 8, 2021

ChengYanJin Apr 6, 2021

romainlancia Apr 8, 2021

ChengYanJin Apr 6, 2021

romainlancia Apr 8, 2021

ChengYanJin Apr 6, 2021

romainlancia Apr 8, 2021

bert-e commented Apr 6, 2021

bert-e commented Apr 8, 2021

romainlancia commented Apr 8, 2021 •

edited

Loading

romainlancia commented Apr 19, 2021

		@@ -101,86 +99,125 @@ const Caret = styled.span`

		const TriggerStyled = ButtonStyled.withComponent("div");

		const DropdownTriggerContainer = forwardRef<DropdownTriggerContainerProps, Element>(({isItem, dataIndex, open, size , variant, title, onBlur, onFocus, onClick, onMouseEnter, onMouseLeave, children, ...rest}, ref) => {

feat: add possibility nested dropdown #290

feat: add possibility nested dropdown #290

Conversation

romainlancia commented Apr 6, 2021 • edited Loading

bert-e commented Apr 6, 2021

Hello lebroz,

bert-e commented Apr 6, 2021

Waiting for approval

JBWatenbergScality left a comment • 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

romainlancia Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bert-e commented Apr 6, 2021

Waiting for approval

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

bert-e commented Apr 6, 2021

Waiting for approval

bert-e commented Apr 8, 2021

Waiting for approval

romainlancia commented Apr 8, 2021 • edited Loading

romainlancia commented Apr 19, 2021

romainlancia commented Apr 6, 2021 •

edited

Loading

JBWatenbergScality left a comment •

edited

Loading

romainlancia Apr 8, 2021 •

edited

Loading

romainlancia commented Apr 8, 2021 •

edited

Loading