ENH Add action dropdown. Move delete button. #345
Conversation
| const { link } = this.props; | ||
|
|
||
| // TODO Replace with something more unique than a class name, perhaps add an ID? | ||
| if (event.target.className === 'action-menu__toggle btn btn--no-text btn--icon-xl font-icon-dot-3 btn btn-secondary') { |
There was a problem hiding this comment.
To start with you could check whether the target's classList contains action-menu__toggle
There was a problem hiding this comment.
I think we did something similar with the Delete button - you should maybe check that. I think it has some code in the click handler for the delete button that stops propagation so the click event doesn't bubble to the parent component as well
There was a problem hiding this comment.
I think the issue here is that we can't easily alter the click handler for the toggle button. That's taken care of by the ActionMenu component as a part of admin. Perhaps that's something to add to silverstripe/silverstripe-admin#610
There was a problem hiding this comment.
I see. Maybe the click handler needs a callback prop you can pass in e.g. beforeHandleClick which is given the event argument so we can tell it to stop propagation
| /> | ||
| <div className="element-editor-header__actions"> | ||
| <ActionMenuComponent | ||
| id={this.props.id} |
There was a problem hiding this comment.
Should we prefix this with something unique to the actions menu maybe like element-editor-actions-${id} or something? Note id is destructured already at this point
There was a problem hiding this comment.
Good point. Changed it as suggested.
There was a problem hiding this comment.
Also FYI - IDs that start with a number is invalid HTML!
raissanorth
force-pushed
the
pulls/master/add-actiondropdown
branch
from
503d34f to
a8f0795
Compare
| @@ -20,7 +20,7 @@ class Element extends Component { | |||
| /** | |||
| * Take the user to the GridFieldDetailForm to edit the record | |||
| */ | |||
| handleClick() { | |||
There was a problem hiding this comment.
We don't need this any more =)
There was a problem hiding this comment.
Do you say that because this functionality is moving into the caret button in the header? And we don't want to replicate this if the user clicks anywhere on the element?!
There was a problem hiding this comment.
I think it's mentioning that you've added event but you're not using it anymore 🙂
There was a problem hiding this comment.
ah, i removed that since - locally. will be in the next rebase.
raissanorth
force-pushed
the
pulls/master/add-actiondropdown
branch
from
a8f0795 to
6ffde24
Compare
Partially resolves #322
The ACs for the above ticket include styling. Outstanding is the amendment of the size of the toggle button icon, which depends on silverstripe/silverstripe-admin#610 (review), so I'll make a seperate PR for this.
Changes in this PR:
Depends on silverstripe/silverstripe-admin#610 to prevent the delete button events to bubble up the element itself (the latter would cause a redirect to the edit page)