Skip to content
This repository has been archived by the owner on Mar 30, 2018. It is now read-only.

change dropdown markup #478

Merged
merged 25 commits into from
Mar 22, 2017
Merged

change dropdown markup #478

merged 25 commits into from
Mar 22, 2017

Conversation

sureshHARDIYA
Copy link
Contributor

@sureshHARDIYA sureshHARDIYA commented Mar 17, 2017

Fixes #465

@stipsan
Copy link
Owner

stipsan commented Mar 17, 2017

Storybook Hub is building storybooks for every commit in this PR.

Latest Storybook: https://90273f4f-bd6d-4b4d-86e5-46ecbc55171f.sbook.io/
( Visit here for more info )

This comment is automatically added by Storybook Hub

@sureshHARDIYA sureshHARDIYA changed the title DRAFT: change dropdown markup change dropdown markup Mar 20, 2017
Copy link
Owner

@stipsan stipsan left a comment

Choose a reason for hiding this comment

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

Instead of duplicating and complicating logic this should be made a lot easier to use.

It shouldn't be harder to use than:

const MyTarget = (props) => <a {...props} className={cx('my-awesome-target', props.className)}>My Target</a>

const MyDropdown = (props) => <div {...props}>My content</div>

const App = () => (
  <Dropdown>
    <MyTarget />
    <MyDropdown />
  </Dropdown>
)

It should also be possible to do this:

const App = () => (
  <Dropdown>
    {({open, onClick}) => (
      <div onClick={onClick}>
         <a className={cx({'uk-open': open})}>Target</a>
          <div className={cx('uk-navbar-dropdown', {'uk-open': open})}>
            Dropdown content here
          </div>
      </div>
    )}
  </Dropdown>
)

src/Dropdown.js Outdated
'uk-open': isOpen,
})
const DropdownProps = {

const dropdownClassName = cx('uk-dropdown uk-dropdown-bottom-left', {
Copy link
Owner

Choose a reason for hiding this comment

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

The default classname here .uk-dropdown .uk-dropdown-bottom-left must be made configurable using a prop.

src/Dropdown.js Outdated
const { isOpen } = this.state
const className = cx(this.props.className, {
const className = cx('uk-button uk-button-default', {
Copy link
Owner

Choose a reason for hiding this comment

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

This should not be hardcoded

src/Dropdown.js Outdated
link: PropTypes.string,
}

static defaultProps = {
dropdownIcon: '',
Copy link
Owner

Choose a reason for hiding this comment

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

I can't see this prop used anywhere?

Copy link
Contributor Author

@sureshHARDIYA sureshHARDIYA Mar 20, 2017

Choose a reason for hiding this comment

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

dropdownIcon, a remainder that we need this props. Could be added once we have SVG issues done.

Copy link
Owner

Choose a reason for hiding this comment

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

I understand, but we shouldn't add anything that isn't used regardless.

src/Dropdown.js Outdated
onClick: handleClick,
onMouseEnter: mode === 'hover' && handleMouseEnter,
onMouseLeave: mode === 'hover' && handleMouseLeave,
children: this.props.dropdownLabel || mode,
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand the logic here, why is mode passed as children?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the label for button.
screen shot 2017-03-20 at 15 28 13

So, if user does not define the dropdownLabel, it will just use mode text. The button will say hover or click on the label.

Copy link
Owner

Choose a reason for hiding this comment

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

The core component shouldn't have default props or behaviour that is only useful in a demo/story.

src/Dropdown.js Outdated
'aria-haspopup': true,
className: link !== 'navbar' ? className : '',
onClick: handleClick,
onMouseEnter: mode === 'hover' && handleMouseEnter,
Copy link
Owner

Choose a reason for hiding this comment

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

it is not optimal to duplicate event handlers on each of the child components.

@sureshHARDIYA
Copy link
Contributor Author

const MyTarget = (props) => <a {...props} className={cx('my-awesome-target', props.className)}>My Target</a>

const MyDropdown = (props) => <div {...props}>My content</div>

const App = () => (
  <Dropdown>
    <MyTarget />
    <MyDropdown />
  </Dropdown>
)

Not sure, how logic for mouseEnter and mouseEnter will be passed to MyTarget and MyDropdown, commits would nice.

@stipsan
Copy link
Owner

stipsan commented Mar 20, 2017

Same way you pass props to the target in the Modal component

@stipsan stipsan closed this Mar 20, 2017
@stipsan stipsan reopened this Mar 20, 2017
@sureshHARDIYA sureshHARDIYA merged commit 00b1af8 into v3 Mar 22, 2017
@sureshHARDIYA sureshHARDIYA deleted the upgrade/dropdown branch March 22, 2017 09:56
stipsan added a commit that referenced this pull request Sep 30, 2017
* fixing the navbar and updating the logo to match v3 (#480)

* fixing the navbar and updating the logo to match v3

* fix link

* Revert "Revert "Merge branch 'v3' into master"" (#493)

966dc20

* change dropdown markup (#478)

* change dropdown markup

* Add extra storybook

* Add props to pass to button

* update the dropdown component proposition

* remove mini component from Input and Button test

* update the tests

* rename some props

* update the test

* update stories

* update the component

* update the tests

* add more tests

* Fix removal of Button stories

* add more tests

* fix linting

* refactor the test

* fix the tests

* update dropdown

* update dropdown SANPSHOTS

* update tests

* make coverage 100% again

* make coverage 100% again

* make coverage 100% again

* DRAFT EC-2549 icon system (#495)

* Working on the icon system

* Still working on the icon

* icon render is possible

* do not have duplicate <svg> tags

* fix the linting

* make coverage 100% again

* update tests

* remove extra Modal stories

* Prerelease/3.0.0 (#501)

* Update less-loader to the latest version 🚀 (#496)

* chore(package): update less-loader to version 4.0.2

https://greenkeeper.io/

* chore: update yarn.lock

* beta release

* cleanup how the css is loaded (#504)

* cleanup how the css is loaded

* file not needed

* Feature/EC-497-Icons fix (#509)

* Add icons in uikit-form

* add support for Icons

* add spinner

* update snapshot

* update Icon tests

* update snapshot

* update the package.json file to run coverage from src

* update Icon tests

* remove dead code and revert Notification isSticky

* refactor according to review

* chore(package): update version (#510)

* Fixing build size (#511)

* tests and stories should not be included in the npm module

* deprecated

* Update .npmignore

Slimming down the module on npm

* Update .npmignore

Some more things that should not be published to npm

* Update .npmignore

* Update .npmignore

* Update Logo.js

better height

* chore(package): update version (#512)

* Feature/EC-2639 more svg icons (#515)

* move icon to folder

* working on icon loading

* working on special case icons

* preserves original viewBox

* test coverage

* Update utils.js

* Prerelease/3.0.0 beta.3 (#517)

* Update README.md (#513)

* chore(package): update style-loader to version 0.16.1 (#514)

https://greenkeeper.io/

* chore(package): update version

* Update uikit to the latest version 🚀 (#522)

* chore(package): update uikit to version 3.0.0-beta.19

https://greenkeeper.io/

* grab junk before svg tag

* Updating snapshots

* Upgrade dependencies (#569)

* upgrade some dependencies

* import PropTypes from prop-types

* upgrade uikit version

* remove react-collapse as we are not using anywhere, @stipsan says, probably a leftover we can remove

* Turn off uncool rules

* Remove  uncool commented Modal component that won't be added in long run

* Yo bro, you need prop-types shorted,

* make lint happy

* Temporary work around

why?

* revert eslint upgrade

* Prerelease/3.0.0 beta.4 (#570)

* chore(package): update react-height to version 3.0.0

https://greenkeeper.io/

* chore(package): update react-collapse to version 3.2.2

https://greenkeeper.io/

* chore(package): update react-collapse to version 3.3.0

https://greenkeeper.io/

* chore(package): update react-collapse to version 3.3.1

https://greenkeeper.io/

* chore(package): update react-collapse to version 4.0.1

https://greenkeeper.io/

* chore(package): update babel-eslint to version 7.2.3

https://greenkeeper.io/

* chore(package): update jsdom to version 10.0.0

https://greenkeeper.io/

* chore(package): update mocha to version 3.3.0

https://greenkeeper.io/

* chore(package): update react-motion to version 0.5.0

https://greenkeeper.io/

* chore(package): update react-collapse to version 4.0.2

https://greenkeeper.io/

* chore(package): update coveralls to version 2.13.1

https://greenkeeper.io/

* fix(package): update react-input-autosize to version 1.1.2

https://greenkeeper.io/

* fix(package): update react-input-autosize to version 1.1.3

https://greenkeeper.io/

* chore(package): update jsdom to version 10.1.0

https://greenkeeper.io/

* chore(package): update style-loader to version 0.17.0

https://greenkeeper.io/

* chore(package): update css-loader to version 0.28.1

https://greenkeeper.io/

* import PropTypes from 'prop-types` (#558)

* import PropTypes from 'prop-types`

* upgrade react-portal

* chore(package): update babel-jest to version 20.0.0

* chore(package): update autoprefixer to version 7.0.1

* chore(package): update postcss-loader to version 2.0.0

* chore(package): update postcss-loader to version 2.0.1

* chore(package): update babel-jest to version 20.0.1

* merge conflict

* chore(package): update version

* we need yarn-update-snapshot

* Hotfix/drop react input autosize (#571)

* should have been removed long ago

* upgrade yarn file

* chore(package): update version (#572)

* chore(package): update version (#574)

* Update changelog

* Remove guard.js to allow merging v3 to master

* fix duplicate imports

* update lockfile

* Removed guard.js
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants