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

Abstract Editor Control #216

Merged
merged 19 commits into from Aug 18, 2016

Conversation

Projects
None yet
3 participants
@sayedtaqui
Collaborator

sayedtaqui commented Aug 9, 2016

No description provided.

@sayedtaqui sayedtaqui changed the title from [WIP] Abstract Editor Control to Abstract Editor Control Aug 12, 2016

@sayedtaqui

This comment has been minimized.

Show comment
Hide comment
@sayedtaqui

sayedtaqui Aug 12, 2016

Collaborator

@westonruter The PR is ready for review. I have tested it with multiple editor controls combinations and did not find any issue. I tried to configure pre-commit hook but could not do it correctly, so sorry for the failed checks.

Collaborator

sayedtaqui commented Aug 12, 2016

@westonruter The PR is ready for review. I have tested it with multiple editor controls combinations and did not find any issue. I tried to configure pre-commit hook but could not do it correctly, so sorry for the failed checks.

label: postTypeObj.labels.content_field ? postTypeObj.labels.content_field : api.Posts.data.l10n.fieldContentLabel,
active: true,
setting_property: 'post_content',
settings: {
'default': setting.id

This comment has been minimized.

@westonruter

westonruter Aug 16, 2016

Contributor

I want to see what possibilities there are for eliminating setting_property in favor of:

settings: {
    'default': setting.id + '[post_content]'
}

That would seem more elegant, but I think it will require a core change to support.

@westonruter

westonruter Aug 16, 2016

Contributor

I want to see what possibilities there are for eliminating setting_property in favor of:

settings: {
    'default': setting.id + '[post_content]'
}

That would seem more elegant, but I think it will require a core change to support.

westonruter added some commits Aug 17, 2016

Rename editor to post_editor; implement custom control template
* Implement longstanding todo to improve how editor control extends the dynamic control.
* Move api.EditorControl to api.Posts.PostEditorControl
* Remove need to inject button since now part of template
* Eliminate use of propertyElements in favor of directly mutating the setting value.
* Eliminate isMeta in favor of being agnostic about what kind of setting is being modified.
* Store a reference to the main textarea and re-use.
@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Aug 18, 2016

Contributor

@sayedwp this is ready for re-review. Could you give it a try with your integration?

Contributor

westonruter commented Aug 18, 2016

@sayedwp this is ready for re-review. Could you give it a try with your integration?

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Aug 18, 2016

Contributor
Contributor

westonruter commented Aug 18, 2016

@sayedtaqui

This comment has been minimized.

Show comment
Hide comment
@sayedtaqui

sayedtaqui Aug 18, 2016

Collaborator

@westonruter Tested with my integration , works beautifully. 👍

Collaborator

sayedtaqui commented Aug 18, 2016

@westonruter Tested with my integration , works beautifully. 👍

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Aug 18, 2016

Contributor

@sayedwp I have a proposal for something which could help disambiguate when there are multiple fields that could have an editor open:

customize__wordpress_develop_5

This fixes something I noticed where I would forget what the open editor was for. If you like it, would you apply some style improvements?

Contributor

westonruter commented Aug 18, 2016

@sayedwp I have a proposal for something which could help disambiguate when there are multiple fields that could have an editor open:

customize__wordpress_develop_5

This fixes something I noticed where I would forget what the open editor was for. If you like it, would you apply some style improvements?

@sayedtaqui

This comment has been minimized.

Show comment
Hide comment
@sayedtaqui

sayedtaqui Aug 18, 2016

Collaborator

@westonruter I was just going to say that, and I saw your commit 😄
I had given it a thought even before, when I was going to toggle the editor animation class. I was thinking to give it tabs ( only if there are more than one editor controls) just like browser tabs. But then it will take some 50-60px of space above the editor.

Collaborator

sayedtaqui commented Aug 18, 2016

@westonruter I was just going to say that, and I saw your commit 😄
I had given it a thought even before, when I was going to toggle the editor animation class. I was thinking to give it tabs ( only if there are more than one editor controls) just like browser tabs. But then it will take some 50-60px of space above the editor.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Aug 18, 2016

Contributor

Probably shouldn't increase the overall height any. If the title is within the bar, could maybe some gray borders on either side make it feel more natural? Maybe tabs in the future of we allow more than one editor to be expanded at once.

Contributor

westonruter commented Aug 18, 2016

Probably shouldn't increase the overall height any. If the title is within the bar, could maybe some gray borders on either side make it feel more natural? Maybe tabs in the future of we allow more than one editor to be expanded at once.

@sayedtaqui

This comment has been minimized.

Show comment
Hide comment
@sayedtaqui

sayedtaqui Aug 18, 2016

Collaborator

Agree on not increasing the overall height. Will add some css to make it look natural.

Collaborator

sayedtaqui commented Aug 18, 2016

Agree on not increasing the overall height. Will add some css to make it look natural.

@sayedtaqui

This comment has been minimized.

Show comment
Hide comment
@sayedtaqui

sayedtaqui Aug 18, 2016

Collaborator

@westonruter Is this looking good?

editor

The icons shows that its a standard post, as shown in post editor page

post-editor-page

Collaborator

sayedtaqui commented Aug 18, 2016

@westonruter Is this looking good?

editor

The icons shows that its a standard post, as shown in post editor page

post-editor-page

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Aug 18, 2016

Contributor
Contributor

westonruter commented Aug 18, 2016

@sayedtaqui

This comment has been minimized.

Show comment
Hide comment
@sayedtaqui

sayedtaqui Aug 18, 2016

Collaborator

Yes, didn't think of that. And that also makes me think that why you have renamed it to "Post Editor Control" from "Editor Control"? I know it would primarily be used for post content however it would also be used for meta

Collaborator

sayedtaqui commented Aug 18, 2016

Yes, didn't think of that. And that also makes me think that why you have renamed it to "Post Editor Control" from "Editor Control"? I know it would primarily be used for post content however it would also be used for meta

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Aug 18, 2016

Contributor

@sayedwp good point. I guess the main reason why I went with “Post” editor is that “postmeta” is still specific to posts. Also, there is a specific integration with posts in the control (the control is presumed to be added to a Post section):

/**
* Unlink the editor from this post and collapse the editor when the section is collapsed.
*/
section.expanded.bind( function( expanded ) {
if ( expanded ) {
api.Posts.postIdInput.val( section.params.post_id || false );
} else {
api.Posts.postIdInput.val( '' );
control.expanded.set( false );
}
} );

Contributor

westonruter commented Aug 18, 2016

@sayedwp good point. I guess the main reason why I went with “Post” editor is that “postmeta” is still specific to posts. Also, there is a specific integration with posts in the control (the control is presumed to be added to a Post section):

/**
* Unlink the editor from this post and collapse the editor when the section is collapsed.
*/
section.expanded.bind( function( expanded ) {
if ( expanded ) {
api.Posts.postIdInput.val( section.params.post_id || false );
} else {
api.Posts.postIdInput.val( '' );
control.expanded.set( false );
}
} );

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Aug 18, 2016

Contributor

Also, I think there should be a more generic EditorControl as well, eventually. Perhaps we can eventually make PostEditorControl extend this base EditorControl.

Contributor

westonruter commented Aug 18, 2016

Also, I think there should be a more generic EditorControl as well, eventually. Perhaps we can eventually make PostEditorControl extend this base EditorControl.

@sayedtaqui

This comment has been minimized.

Show comment
Hide comment
@sayedtaqui

sayedtaqui Aug 18, 2016

Collaborator

That would make more sense.

Collaborator

sayedtaqui commented Aug 18, 2016

That would make more sense.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Aug 18, 2016

Contributor

@sayedwp cool, once you've pushed up the style changes to the editor title, I think this is good to merge.

Contributor

westonruter commented Aug 18, 2016

@sayedwp cool, once you've pushed up the style changes to the editor title, I think this is good to merge.

@sayedtaqui

This comment has been minimized.

Show comment
Hide comment
@sayedtaqui

sayedtaqui Aug 18, 2016

Collaborator

@westonruter Great, lets merge it.

Collaborator

sayedtaqui commented Aug 18, 2016

@westonruter Great, lets merge it.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Aug 18, 2016

Contributor

@sayedwp I did some more cleanup of the JS and added a feature where the new editor heading will only show up if there is more than one editor control in a section. If there is only the content editor control, then there would be no need to disambiguate with other controls in the section.

Contributor

westonruter commented Aug 18, 2016

@sayedwp I did some more cleanup of the JS and added a feature where the new editor heading will only show up if there is more than one editor control in a section. If there is only the content editor control, then there would be no need to disambiguate with other controls in the section.

@westonruter westonruter merged commit e647acf into develop Aug 18, 2016

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 96.55%
Details

@westonruter westonruter added this to the 0.8 milestone Aug 18, 2016

@westonruter westonruter deleted the feature/editor-control branch Aug 18, 2016

@valendesigns

This comment has been minimized.

Show comment
Hide comment
@valendesigns

valendesigns Sep 11, 2016

Member

The weird tab you guys added, which I think looks really strange sitting in the middle there, doesn't include mobile styles. So when you scale the browser down you get this.

mobile-style-bug

Member

valendesigns commented Sep 11, 2016

The weird tab you guys added, which I think looks really strange sitting in the middle there, doesn't include mobile styles. So when you scale the browser down you get this.

mobile-style-bug

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Sep 11, 2016

Contributor

@valendesigns the tab is there to give the user context as to what they are editing. It appears when there are multiple editor controls in a given section.

@sayedwp would you update the styling to factor in mobile?

Contributor

westonruter commented Sep 11, 2016

@valendesigns the tab is there to give the user context as to what they are editing. It appears when there are multiple editor controls in a given section.

@sayedwp would you update the styling to factor in mobile?

@sayedtaqui

This comment has been minimized.

Show comment
Hide comment
@sayedtaqui

sayedtaqui Sep 12, 2016

Collaborator

Sure I will update that soon.

Collaborator

sayedtaqui commented Sep 12, 2016

Sure I will update that soon.

@valendesigns

This comment has been minimized.

Show comment
Hide comment
@valendesigns

valendesigns Sep 12, 2016

Member

@westonruter I understand why it was added but from a UX perspective it's misleading. It's not a tab at all. It doesn't bind events or navigate to anything. So to clear up context we've introduced confusion elsewhere. I think it would be better if we removed the tab style and just centered the text IMO. Thoughts?

Member

valendesigns commented Sep 12, 2016

@westonruter I understand why it was added but from a UX perspective it's misleading. It's not a tab at all. It doesn't bind events or navigate to anything. So to clear up context we've introduced confusion elsewhere. I think it would be better if we removed the tab style and just centered the text IMO. Thoughts?

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Sep 12, 2016

Contributor

I guess the idea is that the entire editor could be considered a tab and that's why that style was chosen. @sayedwp can you work out some non-tab styling for the heading?

Contributor

westonruter commented Sep 12, 2016

I guess the idea is that the entire editor could be considered a tab and that's why that style was chosen. @sayedwp can you work out some non-tab styling for the heading?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment