-
Notifications
You must be signed in to change notification settings - Fork 31
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
Use the fixed toolbar for hallo #16
Conversation
Also, prevent the br tag from being used in titles
@@ -4,7 +4,7 @@ | |||
{% javascripts output="js/hallo-coffee.js" | |||
'@SymfonyCmfCreateBundle/Resources/public/js/init-create-hallo.js' | |||
'@SymfonyCmfCreateBundle/Resources/public/vendor/hallo/src/hallo.coffee' | |||
'@SymfonyCmfCreateBundle/Resources/public/vendor/hallo/src/toolbar/*.coffee' | |||
'@SymfonyCmfCreateBundle/Resources/public/vendor/hallo/src/toolbar/fixed.coffee' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure about this change? the other coffee files there are not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the toolbar folder there's only one other coffee file, which is for the current position. I think we could load both of them but it's not necessary.
'hallolists': {'lists': {'ordered': false}}, | ||
'hallojustify': {}, | ||
'hallotoolbarlinebreak': { 'breakAfter': ['hallolink'] }, | ||
'hallooverlay': {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from the hallo source code i see it still has this widget. can we not fix it in the sandbox to make it work again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overlay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we had One to darken everything that is not the current editable area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, do we explicitly want it though, or is it a case of "it was there before, why isn't it now"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the UX people wanted to do it that way. and i think it indeed made things look nicer and easier to use. if its not too hard to fix, would be nice to get it back. if its really a big pain we should open a new issue for it to not block this PR forever.
@colinfrei thanks for attempting to clean up things, most of it is good as is. can we do something about the overlay? the main question now is: why do you want to use the header toolbar now? to avoid the scrolling issue? or are there other arguments? |
@dbu could you screenshot things a bit to get an idea of the choice you're facing? I didn't follow recent works. |
the toolbar next to text is visible here when you click into the text http://cmf.liip.ch for the new layout i send you an email with a screenshot |
@colinfrei i just noticed that the image dialog popup does not really work from the header toolbar. the top is hidden by the create toolbar, so i can not switch the tab and neither move or close it without first reducing the create toolbar. i think it should be positioned under the create toolbar. |
btw i have updated create.js/hallo.js to the lastest version. |
@colinfrei can you have a look at this tomorrow? |
ok, so what is left:
|
New status for this PR:
Should we merge? |
Also, prevent the br tag from being used in titles