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

EZP-24829: Change the display of the richtext focus mode #440

Merged
merged 1 commit into from Nov 26, 2015

Conversation

5 participants
@StephaneDiot
Copy link
Contributor

StephaneDiot commented Nov 20, 2015

https://jira.ez.no/browse/EZP-24829

Description

Changing the display of the richtext edit field focus mode according to the spec (https://doc.ez.no/display/PR/Online+Editor%3A+Scope#OnlineEditor:Scope-Story2:Asaneditor,Iwanttoeditelementsinexistingeditorialcontent)

Screenshot

image

Tests

unit and manually tested

@StephaneDiot StephaneDiot force-pushed the StephaneDiot:EZP-24829_Change_the_display_of_the_richtext_focus_mode branch from 66c8bae to e2acc1f Nov 20, 2015

@StephaneDiot

This comment has been minimized.

Copy link
Contributor Author

StephaneDiot commented Nov 20, 2015

@StephaneDiot StephaneDiot changed the title EZP-24829_Change_the_display_of_the_richtext_focus_mode EZP-24829: Change the display of the richtext focus mode Nov 20, 2015

@@ -59,9 +62,6 @@ YUI.add('ez-richtext-editview', function (Y) {

render: function () {
Y.eZ.RichTextEditView.superclass.render.call(this);

This comment has been minimized.

@dpobel

dpobel Nov 20, 2015

Contributor

this method becomes useless, it just calls the parent render.

@@ -100,7 +100,8 @@ YUI.add('ez-richtext-editview', function (Y) {
* @method _unsetFocusMode
* @protected
*/
_unsetFocusMode: function () {
_unsetFocusMode: function (e) {

This comment has been minimized.

@dpobel

dpobel Nov 20, 2015

Contributor

the description in doc block is outdated and the parameter is not documented

display: inline-block;
}

.ez-view-richtexteditview.is-focused .ez-editactionbar-container {

This comment has been minimized.

@dpobel

dpobel Nov 20, 2015

Contributor

The RichText edit view does not generate such .ez-editactionbar-container ?

@StephaneDiot StephaneDiot force-pushed the StephaneDiot:EZP-24829_Change_the_display_of_the_richtext_focus_mode branch from e2acc1f to b92fed0 Nov 20, 2015

button.simulateGesture('tap');
this.wait();
},

"Should disbale the focus mode on saveReturnAction event": function () {

This comment has been minimized.

@dpobel

dpobel Nov 20, 2015

Contributor

this one should be removed, I'm also surprised it's still passing...

@@ -64,7 +78,7 @@

.ez-view-richtexteditview.is-focused .ez-editfield-row {
padding-left: 0;
padding-right: 230px;
padding-right: 0;

This comment has been minimized.

@dpobel

dpobel Nov 20, 2015

Contributor

you can even remove that

var that = this,
button = this.view.get('container').one('.ez-richtext-save-and-return');

this.view.set('focusMode', true);

This comment has been minimized.

@dpobel

dpobel Nov 20, 2015

Contributor

focusMode is read only, so you have to use _set to really its value.

@StephaneDiot StephaneDiot force-pushed the StephaneDiot:EZP-24829_Change_the_display_of_the_richtext_focus_mode branch from b92fed0 to b2d0db1 Nov 20, 2015

@dpobel

This comment has been minimized.

Copy link
Contributor

dpobel commented Nov 20, 2015

besides the CS nitpick, +1

@StephaneDiot StephaneDiot force-pushed the StephaneDiot:EZP-24829_Change_the_display_of_the_richtext_focus_mode branch from b2d0db1 to fcdb073 Nov 20, 2015

@vidarl

This comment has been minimized.

Copy link
Member

vidarl commented Nov 23, 2015

FYI : There was some problem running the "ez/ci/behat" ezrobot for commit fcdb073. The problem was likely fixed by a upgrade of docker-compose. Add another change to the PR to kickstart a new test execution

@dpobel

This comment has been minimized.

Copy link
Contributor

dpobel commented Nov 23, 2015

Add another change to the PR to kickstart a new test execution

no, the code is correct, there's absolutely no reason to change a correct code. CI should provide a restart button like Travis... no actually we should use Travis for that, at least we would have the restart button "for free" and the result would be public...

@mhyndle

This comment has been minimized.

Copy link
Contributor

mhyndle commented Nov 25, 2015

+1

StephaneDiot added a commit that referenced this pull request Nov 26, 2015

Merge pull request #440 from StephaneDiot/EZP-24829_Change_the_displa…
…y_of_the_richtext_focus_mode

EZP-24829: Change the display of the richtext focus mode

@StephaneDiot StephaneDiot merged commit aa072be into ezsystems:master Nov 26, 2015

2 of 3 checks passed

ez/ci/behat Behat test execution by ezrobot
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
ezrobot Code review by ezrobot
Details
@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Nov 26, 2015

no actually we should use Travis for that

travis was not able to run the UI.

at least we would have the restart button "for free" and the result would be public...

it is public now

@dpobel

This comment has been minimized.

Copy link
Contributor

dpobel commented Nov 27, 2015

travis was not able to run the UI.

Out of curiosity, I'd really like to know what you can't do on Travis since you basically get a VM with full access to install anything like a browser and xvfb for instance ?

it is public now

That's a great step forward. Can I haz a restart button at some point ? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.