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.

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

Toolbar install/upgrade bug fixes #247

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Toolbar install/upgrade bug fixes #247

wants to merge 19 commits into from

Conversation

techanon
Copy link
Contributor

Continuing from #236.

-> Fixed incorrect handler link for tools needing upgrade
-> Fixed "Click to X" duplication bug
-> Added prevention of admin/upgrade being shown in the toolbar editor
-> Added CSS to the toolbar editor to signify if an app requires action taken

Let me know if you wanna have a different label for the admin/upgrade tool.

@jbroadway
Copy link
Owner

I made some changes and I think I left this PR in a partially merged state. Can you fetch the latest from the main repo and help sort that out?

I did make a few stylistic changes:

  • The Edit Toolbar is in the top right and appears purple
  • I made the Extras styling consistent with the other headers and added some explanatory text to it
  • I removed the (app/handler) from the resources and made their URLs appear on hover via the title attribute, e.g., /admin/pages
  • I removed styling uninstalled apps differently in the editor. Since you can't click on them from there, I think that may cause confusion.

@techanon
Copy link
Contributor Author

I'm not sure if you meant keep the Edit Toolbar text to the right or not, so I pushed up what I had for that atm. I can revert that if you wish (though I think the text works better being centered).

Also, note: The old toolbar dropdown layout is self-depreciating due to the fact that any change to the toolbar creates the tools.php file (if it doesn't exist already) that flags the layout as being the new format. I think it'd work best if the fallback (old layout) was left in for the next release, and then removed for the one following.

@techanon
Copy link
Contributor Author

Bump

@jbroadway
Copy link
Owner

I did style and move the edit link to the right on purpose. Something about centering it looked off to me...

Thinking about the fallback mode, I'm not sure we need it at all since new releases already include a default tools.php file, and IMO the new menu is much nicer now too.

@techanon
Copy link
Contributor Author

Done. (Also, might want to check the Travis build... seems a file or class is missing from the tests for php7)

@jbroadway
Copy link
Owner

I'm a bit confused since this was partially merged before and seems to be working well from what I can tell. Can you let me know what the differences are or what's still outstanding? Thanks :)

Not sure what's up with the PHP7 issue yet, seems to be that PHPUnit moved some things to external packages and a simple pear install didn't fix it. Still investigating that one!

…ries

Added hardcoded toolbar defaults
Changed the action taken when a tools file is unavailable to use, it now generates the file from the defaults
Removed the default tools reference file since they are now hardcoded
@techanon
Copy link
Contributor Author

With the removal of the fallback, if there is no tools file avaliable, it will generate the tools.php file from hardcoded defaults. I also found a bug in the tree-drag-drop script.
I'm probably going to find something after I say this, but I'm pretty sure that's everything for the toolbar.

@techanon
Copy link
Contributor Author

Did some more digging around in this PR and haven't found any other bugs needing squashed. Is there anything else that the rest of this PR needs done or explained?

@techanon
Copy link
Contributor Author

techanon commented Jul 6, 2015

Merged the master branch in. Pardon the git stupidity, there was a local file permissions issue that git was complaining about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants