epsacrop.module @ln 446

          '#markup' => "<a href=\"javascript:Drupal.EPSACrop.dialog('" . $element['#entity_type'] . "', '" . $element['#field_name'] . "', '" . $element['#bundle'] . "', '" . $file->fid . "', '" . $url . "', [" . $info['width'] . "," . $info['height'] . "]);\" class=\"button espacrop\">" . t("manage image crops") . "</a>"

Great module. I'm submitting a quick UX change so that the buttons match the rest of drupals call to actions. Hopefully you find it worth committing. Thanks for your work on this. Its a handy little tool. ; )

--
J.

CommentFileSizeAuthor
#4 button_container-1459840-3.patch974 bytesaacraig
#1 button_container-1459840.patch985 bytesaacraig

Comments

aacraig’s picture

aacraig commented
StatusFileSize
new button_container-1459840.patch985 bytes

I've rolled a patch against the current dev version. It's a bit different (wrapping the link in a

rather than adding a class and adjusting the position of the [] characters) but the end result is the same.
keesee’s picture

keesee commented

Thanks for rolling a patch aacraig.... however the main intent behind my recommendation was the addition of a button class to the link, which you excluded. it should be a button. and other modules have adopted this. It keeps themers from having a zillion lines of css. its also attempting to introduce consistency as I've have this request of several modules that haven't adopted a.button. I also don't see a huge need for the containing div as the other elements in the parent div.image-widget-data dont have containers including input.remove.

class="button espacrop" -- conforms to a button class yet allows overriding via a.button.espacrop.

I'll reroll a patch later this week with some other git tasks I have. Don't have my git tools on this tablet. Thanks again for the patch. Appreciated. In the future though, unless you are the module maintainer please roll patches out for users as they intended or at the very least include there suggestions with your own to keep from duplicating efforts.

keesee’s picture

keesee commented
Status: Needs review » Needs work
aacraig’s picture

aacraig commented
StatusFileSize
new button_container-1459840-3.patch974 bytes

My bad -- I didn't realize that a.button was becoming standard practice.

I've rerolled the patch with the changes as you've suggested. I've also removed the [ and ] characters from the beginning and end of the link text, so the text looks right in a button.

keesee’s picture

keesee commented

Awesome. aacraig.
Thank you. looks great.

I'm rolling out an administrative theme for D7 and have been testing. Working well and I've been submitted code changes to modules I use that don't already have it. I can't remember the source, somewhere in the d7ux or d8ux notes... buit I am seeing it more regularly.

Makes good sense to add it. give you a ton of options ranging from the example above of a.button.espacrop. to a.button.cancel etc.

Thanks again. Hopefully they'll commit it. ; )
--
j.

aacraig’s picture

aacraig commented

Yeah -- good call on that one. Happy to have had the opportunity to learn something new :)

yvmarques’s picture

yvmarques commented
Status: Needs work » Closed (fixed)