Feature/popover #13

swapnesh · 2019-04-02T04:45:38Z

EDIT 02 -

@vinodloha Please find new changes as requested -

Added BEM naming convention in writing classnames.
Used prop based styling as suggested rather than creation of new nodes.
Changed code structure to use the component body to set the Popover content and defined prop for supplying heading and trigger element/content as suggested.
Did bare minimum css.
Keeping trigger as "React$Element<*>" instead on Node as Node type is incompatible with the elements if cloned
const element = trigger && React.cloneElement(trigger, { onClick: this.handleClick });
Removed unnecessary dummy title Helloooooo from story.js file.

Let me know on case of any further query.

EDIT 01 -

@vinodloha Please find changes made after changing Atom to Molecule Popover. Please let me know in case of further query.

Changed Atom to molecule.
Added isVisible property to have more controlled component.
Added List component to show in the popover body component.
Done with bare minimum CSS part by adding border/colour etc.
Changed header to h3 tag than div to be more semantically correct.
Added close button in Popover.
Changed data types as requested.

New Screenshot -

Initial commit for Atom | Popover.

…y-fix Fixed Atom name hardcoding from story on component basis

vinodloha · 2019-04-02T14:03:23Z

lib/components/atoms/Popover/Popover.js

@@ -0,0 +1,74 @@
+// @flow


@swapnesh Popover should be a molecule, Since it usually holds and displays other components. Please restructure it.

@vinodloha I made the changes as mentioned. Kindly review

vinodloha · 2019-04-02T14:04:00Z

lib/components/atoms/Popover/Popover.js

+  };
+
+  state: State = {
+    showPopover: false,


Use isVisible property instead.

vinodloha · 2019-04-02T14:07:03Z

lib/components/atoms/Popover/Popover.mock.js

@@ -0,0 +1,7 @@
+const defaultConfig = {


Can you also make a sample where we are putting another component like LIst inside popover?

vinodloha · 2019-04-02T14:08:53Z

lib/components/atoms/Popover/Popover.style.js

+import { css } from 'styled-components';
+
+export default css`
+  ${props => (props.inheritedStyles ? props.inheritedStyles : '')};


Please do right bare minimum CSS like: Border Box, Small padding to all 4 side of the content.

vinodloha · 2019-04-02T14:11:16Z

lib/components/atoms/Popover/Popover.js

+      <div role="presentation" onClick={this.handleClick} ref={this.wrapperRef}>
+        {children}
+        {showPopover && (
+          <div role="dialog">


Pop overs usually have a pointer too can we include that conditionally? Example: https://codepen.io/esaramago/pen/jbXZaz and https://getbootstrap.com/docs/4.1/components/popovers/

Also Placing a class on popOverHeader and popOverBody divs would give better styling control to people using it.

vinodloha · 2019-04-02T14:12:11Z

lib/components/atoms/Popover/Popover.js

+        {children}
+        {showPopover && (
+          <div role="dialog">
+            <div>{popOverHeader}</div>


Semantically DIV doesn't seem right tag for it. May be a H3 will help??

vinodloha · 2019-04-02T14:13:07Z

lib/components/atoms/Popover/Popover.js

+    return (
+      <div role="presentation" onClick={this.handleClick} ref={this.wrapperRef}>
+        {children}
+        {showPopover && (


Can we also have an optional Close button with-in the popover?

vinodloha · 2019-04-02T14:14:45Z

lib/components/atoms/Popover/tests/Popover.test.js

+
+import Popover from '../index';
+
+describe('<Popover />', () => {


The test could be a bit better, at least test the code you wrote, like close and open functionality. Or any other new feature you add. Just snapshot is not enough.

vinodloha · 2019-04-02T14:15:24Z

lib/components/atoms/Popover/types/index.js

+// @flow
+import type { Node } from 'react';
+
+export type Props = { popOverBody: String, popOverHeader: String, children: Node };


This is not how we define type String --> string
I would recommend you change
popOverHeader --> heading (It should be both string and Node type)
popOverBody --> trigger (This should be a Node or String that will trigger popover)
children --> it should be the content or body of the popover

vinodloha · 2019-04-04T12:52:10Z

lib/components/molecules/Popover/types/index.js

+export type Props = {
+  popOverBody: Node | string,
+  popOverHeader: Node | string,
+  children: React$Element<*>,


For Children correct type is Node

Initially I defined children as node (in new use case - its trigger prop) but getting Linting error. On github flow
issue thread I found that if an element is cloned then it should be mentioned as React$Element<*>

vinodloha · 2019-04-04T12:54:05Z

lib/components/molecules/Popover/Popover.story.js

+storiesOf('Molecules', module).addWithChapters('Popover', {
+  chapters: [
+    {
+      title: `Helloooooo`,


Please Put formal Title

vinodloha · 2019-04-04T12:57:16Z

lib/components/molecules/Popover/Popover.story.js

+      title: 'Popover Variations',
+      sections: [
+        {
+          title: '[isVisible=true] variation',


Title Should be contextual and description: Please name this as "By Default Open Variation"

vinodloha · 2019-04-04T12:57:50Z

lib/components/molecules/Popover/Popover.story.js

+          ),
+        },
+        {
+          title: '[isVisible=false] variation',


By Default Close Variation

amrit07

popover should be accessible via keyboard as well. Ref: https://reactjs.org/docs/accessibility.html
Also they should also breakout from a parent with overflow hidden; for e.g carousel. Ref: https://reactjs.org/docs/portals.html

vinodloha · 2019-04-04T12:58:03Z

lib/components/molecules/Popover/Popover.story.js

+          ),
+        },
+        {
+          title: 'List(Component) variation',


List Content Variation

vinodloha · 2019-04-05T02:19:58Z

lib/components/molecules/Popover/Popover.story.js

+          title: 'Optional close button variation',
+          sectionFn: () => (
+            <Popover {...noCloseBtnPopover}>
+              <button>Click to Open</button>


See this is not really right, the content which is less is placed in the body of component (being picked as children) and the content which really is body gets set as prop. This is a wrong design pattern and would always confuse react developers. Can you please change it, i had given this comment earlier too.

So use the component body to set the Popover content and defined prop for supplying heading and trigger element/content.

@vinodloha Changed the structuring as requested. Now the inner part of component represents the body of popover and trigger prop will represent the node for triggering the body content.

vinodloha · 2019-04-05T02:24:24Z

lib/components/molecules/Popover/Popover.style.js

@@ -0,0 +1,18 @@
+import styled from 'styled-components';
+
+const PopoverContentWrapper = styled.div`


This is not the right way, just encapsulate all CSS in a class and apply that to whole component. Its easier that way to make variations.

vinodloha · 2019-04-05T02:26:44Z

lib/components/molecules/Popover/types/index.js

+  popOverHeader: Node | string,
+  children: React$Element<*>,
+  isVisible: boolean,
+  wrapperBackground: string,


wrapperBackground is useless, if someone wants to overwrite this component's style its not going to be background only. We should make way for enabling them to overwrite all CSS. Set class names in BEM terminology.

We are using this technique ${props => (props.inheritedStyles ? props.inheritedStyles : '')}; in all components. Please follow the same.

Removed the unnecessary class and prop for css and aligned class naming with BEM.

vinodloha · 2019-04-05T02:27:40Z

lib/components/molecules/Popover/tests/Popover.test.js

+    PopoverComponent = shallow(<Popover>Test</Popover>);
+  });
+
+  test('should render correctly', () => {


Test cases needs to improve, you have a open/close functionality too. Test it here!

vinodloha

Please fix these review comments, there are some fundamental problems so can't accept it in current form.

… trigger as element

vinodloha

Looking good now! I have added few extra tests.

But @swapnesh Thanks for this effort, must appreciated :)

vinodloha and others added 2 commits April 2, 2019 10:09

Merge pull request #12 from pagesource/bugfix/scaffold-component-stor…

1f961f4

…y-fix Fixed Atom name hardcoding from story on component basis

Atom | Popover | Initial Commit

2ec80f8

vinodloha requested changes Apr 2, 2019

View reviewed changes

swapnesh added 2 commits April 4, 2019 12:16

Popover | molecule | Changed popover from atom to molecule

4142973

Popover | Molecule | Added conditional close button

edaa07d

vinodloha reviewed Apr 4, 2019

View reviewed changes

amrit07 reviewed Apr 4, 2019

View reviewed changes

vinodloha requested changes Apr 5, 2019

View reviewed changes

vinodloha reviewed Apr 5, 2019

View reviewed changes

vinodloha requested changes Apr 5, 2019

View reviewed changes

swapnesh and others added 4 commits April 8, 2019 11:05

Popover | Molecule | BEM convention for classname and changed styling

efdae07

Popover | Molecule | Changed structuring by inner content as body and…

ec37659

… trigger as element

Merge branch 'develop' into feature/popover

e98968e

Fixed Test Coverage for Popover component and other small lint issues

ee8c906

vinodloha approved these changes Apr 8, 2019

View reviewed changes

vinodloha merged commit 9c7c749 into develop Apr 8, 2019

vinodloha deleted the feature/popover branch April 8, 2019 19:27


		import Popover from '../index';

		describe('<Popover />', () => {

		@@ -0,0 +1,18 @@
		import styled from 'styled-components';

		const PopoverContentWrapper = styled.div`

Feature/popover #13

Feature/popover #13

Uh oh!

Conversation

swapnesh commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amrit07 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinodloha left a comment

Choose a reason for hiding this comment

Uh oh!

vinodloha left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

swapnesh commented Apr 2, 2019 •

edited

Loading