Comments

saltednut’s picture

StatusFileSize
new2.48 KB

Oh - it was missing the invisible wrapper for when edit.css isn't loaded.

saltednut’s picture

StatusFileSize
new3.23 KB

Ugh.. seems I did not include my changes to edit.css in the last patch. :(

steven.wichers’s picture

Status: Needs review » Needs work

Navbar module is using drupal-navbar as the class name. See:

#1757466: Prefix all navbar classes to prevent theme clashes
#1938044: Prefix all toolbar classes to prevent theme clashes

Images also did not get created from the patch file.

saltednut’s picture

@steven.wichers how are you applying the patch? We are using it in the Demo Framework make file and not seeing issues with the images being created.

steven.wichers’s picture

cd path/to/edit
patch -p1 < path/to/patch

saltednut’s picture

@steven.wichers does this work to apply?

rm -r path/to/edit
git clone --branch 7.x-1.x http://git.drupal.org/project/edit.git path/to/edit
cd path/to/edit
git apply --index path/to/patch

steven.wichers’s picture

That worked with the following non-critical warnings:


../../../patches/edit-navbar-button-1994256-2.patch:5: new blank line at EOF.
+
../../../patches/edit-navbar-button-1994256-2.patch:44: new blank line at EOF.
+
warning: 2 lines add whitespace errors.

Must be an issue with patch and creating new files.

Original problem remains: The navbar module is no longer using .navbar.

saltednut’s picture

@steven.wichers No thats just bad patch writing on my part. You can ignore the EOF warnings.

This patch is just a bandaid really. I created it as a stub for the maintainers to hopefully work off of. If you want, please assign the issue to yourself and fix the css errors. Cheers!

saltednut’s picture

StatusFileSize
new3.25 KB

Couldn't wait... :) @steven.wichers this should fix.

Again though, we need to keep working on this so that it works without having the block present on the page.

steven.wichers’s picture

Untested, but from browsing the code it seems as though all that is required is adding '#attached' => array('library' => array(array('edit', 'edit'))) to the page render array.

saltednut’s picture

Status: Needs work » Needs review
StatusFileSize
new1.25 KB
new3.61 KB

Ok, thanks for the tip! I've made some adjustments and this is working on my end even without the 'Quick Edit' block on the page.

saltednut’s picture

StatusFileSize
new2.75 KB
new407 bytes

Oops - title is also required.

saltednut’s picture

StatusFileSize
new3.64 KB

Forgot to add the --binary flag in #12 to include the files. :\

Poieo’s picture

This is working well for me. I think the only thing I would add is:

.drupal-navbar .bar #edit-navbar-tab button:hover {
  cursor: pointer;
}

A little more user friendly.

saltednut’s picture

StatusFileSize
new3.75 KB

@Poieo - that is inconsistent with D8 and Responsive Preview - the button icon should actually turn white on hover:

.drupal-navbar .bar .icon.icon-edit:hover {
  background-image: url("../images/edit-active.png");
}
Poieo’s picture

Hmm... I guess since this is a 'button' and not a link, it's not picking up the css from navbar.theme.css on line 17:

.drupal-navbar a {
    cursor: pointer;
}

I was more concerned with the fact that the cursor doesn't turn to a pointer on desktop devices like the rest of the menu items in navbar do.

saltednut’s picture

That's a good point. Hmm... I was basing this implementation off the Responsive Theme Preview module's implementation of hook_navbar.
-- http://drupalcode.org/project/responsive_preview.git/blob/refs/heads/7.x...

Might be more of a problem for the navbar module to fix. hook_navbar should be adding these classes, no?

greg__’s picture

Can you please attach the image generated by the patch to this issue ?
For an unknown reason, my linux distrib didn't generate them when i applied the patch.

Thank you.

Poieo’s picture

StatusFileSize
new365 bytes

Attached.

wim leers’s picture

Issue summary: View changes
Status: Needs review » Closed (won't fix)

Thanks for working on this, and sorry for not getting back to you sooner.

The Drupal 8 backport (#2111667: Backport Drupal 8's Edit module to Drupal 7) has fixed this — there's no more "Quick edit" link to start in-place editing ALL the things on the page, it now uses contextual links, just like in Drupal 7, hence this is a non-issue now :)

jessebeach’s picture

StatusFileSize
new4.64 KB

I know this issue is closed. I need a place for an interim patch that makes the version of Edit included in the Demo Framework distro work with the latest Navbar code. In other words, just ignore this patch :)

saltednut’s picture

StatusFileSize
new3.95 KB

Yes, its a zombie thread... We continue to roll with an earlier version of Edit in DF due to CKEditor packaging issues. This patch gets us current with Navbar 1.2 for the cf62974 revision of edit. (Please ignore.)

saltednut’s picture

wim leers’s picture

A zombie thread :D

saltednut’s picture

StatusFileSize
new4.12 KB

I think there is still some use case for this. At the very least, could we work on a backport of D8's functionality that highlights contextual links? I'd be happy to open another issue if that is necessary.

From my experience, and in talking to others, we very much found the "Pencil" icon in the navbar as a good place to start for inline editing. I am not sure how much work it would be to either refactor this patch or work on a new solution.

For our purposes, just fixing this patch to work with the latest Edit is a good start. I am attaching a non-working patch here that applies to HEAD. It brings in the navbar button and everything is theme'd correctly. Just asking for some guidance as to what methods to invoke to put this together for demo/PoC purposes.

Wim pointed me to #2172989: Is it possible to print a custom link to quick edit? in chat, so we can use that documentation to work on this PoC button. But it will likely only be in place for editing one node at a time. In the case where multiple entities are shown on a single page, the button will not be available and contextual links will have to be used.

wim leers’s picture

could we work on a backport of D8's functionality that highlights contextual links?

I'm sure it's possible, but it might require overriding all of Contextual Links' JS in D7…