Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(data-table-v2): fix the undefined method called when press in escape key #889

Merged
merged 5 commits into from
Jun 25, 2018
Merged

fix(data-table-v2): fix the undefined method called when press in escape key #889

merged 5 commits into from
Jun 25, 2018

Conversation

rezak-otmani
Copy link
Contributor

@rezak-otmani rezak-otmani commented Jun 13, 2018 •
edited
Loading

Resolves #674

Call _actionBarCancel instead of _batchCancel when the escape key is pressed

Added

Removed

Changed

Testing / Reviewing

asudoh
asudoh reviewed Jun 14, 2018
View reviewed changes
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Thanks alot for jumping in @rezak-otmani! Looking at _selectAllToggle() code, it appears that table row selection has to be updated, too?

@rezak-otmani
Copy link
Contributor Author

Hi @asudoh, thank's you for your return, I cann't see too much the relaton with the function _selectAllToggle()

@asudoh
Copy link
Contributor

asudoh commented Jun 15, 2018

Hi @rezak-otmani let me try to articulate what I meant - IIUC _batchCancel() is for making all table rows unselected. OTOH _selectAllToggle() is for making all table rows selected or all unselected, depending on the checked status of “select all” checkbox. That’s why I tried to do one-to-one comparison between those, and I see we’ll need https://github.com/rezak-otmani/carbon-components/blob/06f7450cb1ec239e6e7cddd839965bad19bfe469/src/components/data-table-v2/data-table-v2.js#L115 line for _batchCancel(), too. Please don’t hesitate to speak up if you think I’m wrong here - Thanks!

@rezak-otmani
Copy link
Contributor Author

Hi @asudoh thanks for your feedback
IIUC the contents of _batchCancel() become the same as the function _actionBarCancel then we can directly call this function instead of declare _batchCancel().

@asudoh
Copy link
Contributor

asudoh commented Jun 19, 2018

Thanks @rezak-otmani thank you for your thoughts here! Using _actionBarCancel() directly makes sense to me.

asudoh
asudoh approved these changes Jun 22, 2018
View reviewed changes
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @rezak-otmani!

@tw15egan tw15egan merged commit 854622a into carbon-design-system:master Jun 25, 2018
@carbon-bot
Copy link
Contributor

🎉 This PR is included in version 9.1.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

joshblack pushed a commit to joshblack/carbon that referenced this pull request May 2, 2019
@kiett @alisonjoseph
feat(OverflowMenu): added onClose & onOpen to props to notify caller …
1fab65e 
…Menu's open state (carbon-design-system#889)

* feat(OverflowMenu): added onClose to props to notify menu is closed

* feat(OverflowMenu): moved onClose handler to componentDidUpdate

* feat(OverflowMenu): moved onClose handler to render() where state.open is surely changed

* feat(OverflowMenu): moved onClose handler to componentDidUpdate

* feat(OverflowMenu): add onOpen to notify caller menu's open state

* feat(OverflowMenu): moved onClose & onOpen handler to componentDidUpdate

* feat(OverflowMenu): added onClose to props to notify menu is closed

* chore(build): fix babel config for test (carbon-design-system#892)

* feat(OverflowMenu): moved onClose handler to componentDidUpdate

* feat(OverflowMenu): moved onClose handler to render() where state.open is surely changed

* feat(OverflowMenu): moved onClose handler to componentDidUpdate

* feat(DatePicker): Added minDate and maxDate props and pass them on to flatpickr (carbon-design-system#887)

* feat(DatePicker): Added minDate and maxDate props and pass them on to flatpickr

* Updated story description

* fix: Pagination component's range and page text for no items (carbon-design-system#872)

* fix: Pagination component's range and page text for no items, resolves carbon-design-system#553

* fix: Update UTs for PaginationV2

* feat(SearchFilterButton): add support for attaching event handlers (carbon-design-system#896)

* fix(DatePicker): treat Flatpickr's default export as a constructor (carbon-design-system#890)

Fixes carbon-design-system#491.

* fix: console.errors coming from using Datepicker (carbon-design-system#899)

* fix(package): update flatpickr to version 4.5.0 (carbon-design-system#897)

* fix(package): update flatpickr to version 4.5.0

Closes carbon-design-system#893

* chore(package): update lockfile

https://npm.im/greenkeeper-lockfile

* fix(table-toolbar-search): add i18n support (carbon-design-system#903)

* fix(OverflowMenu): fix <ClickOutside> not covering floating menu (carbon-design-system#906)

Fixes carbon-design-system#628.

* feat(OverflowMenu): add onOpen to notify caller menu's open state

* feat(OverflowMenu): moved onClose & onOpen handler to componentDidUpdate

* feat(OverflowMenu): moved onOpen call hook to_handlePlace

* feat(OverflowMenu): refine from Josh and Akira's reviews

* feat(OverflowMenu): refine from Josh and Akira's reviews

* feat(OverflowMenu): refine from Josh and Akira's reviews
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asudoh asudoh asudoh approved these changes

@marijohannessen marijohannessen marijohannessen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rezak-otmani @asudoh @carbon-bot @marijohannessen @tw15egan