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

Image Alignment - Center #322

Closed
u2 opened this issue Aug 17, 2015 · 6 comments

Comments

Projects
None yet
3 participants
@u2
Copy link

commented Aug 17, 2015

No description provided.

@u2 u2 changed the title Image Alignment - Left Image Alignment - Center Aug 17, 2015

@ipeychev ipeychev added the question label Aug 17, 2015

@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2015

Hey @u2,

Could you please explain why did you open this issue? Are you going to contribute code or you think this is a missing functionality which should be implemented?

Thanks,

@u2

This comment has been minimized.

Copy link
Author

commented Aug 18, 2015

I think this is a missing functionality which should be implemented.I will try to contribute code, but I am not sure whether I can afford it.

@ejfrancis

This comment has been minimized.

Copy link

commented Oct 5, 2015

taking a quick look at your source of the button-image-align-left.jsx component, it looks like you're creating the style with css and using ButtonActionStyles applyStyle() method in an onclick to add the style to the element with the style defined like so

    getDefaultProps: function() {
            return {
                style: {
                    element: 'img',
                    styles: {
                        'float': 'left'
                    }
                }
            };
        },

if that's all that's necessary (it looks like everythings hooked up in a nice reusable way), adding a button-image-align-center.jsx component should be pretty much the same but with this as default props

    getDefaultProps: function() {
            return {
                style: {
                    element: 'img',
                    styles: {
                        'display': 'block'
                        'margin':'0 auto'
                    }
                }
            };
        },

Am I correct or is there more work required than that? I may play around with it when I get some free time

@ejfrancis

This comment has been minimized.

Copy link

commented Oct 6, 2015

@ipeychev
Follow up, I tried it out and it works fine. added it to ui/react/src/selections/selections.js

 var Selections = [
    ...
     {
        name: 'image',
        buttons: ['imageLeft', 'imageCenter', 'imageRight'],
        test: AlloyEditor.SelectionTest.image
    },
   ...

And added a component ButtonImageAlignCenter

(function () {
    'use strict';

    /**
     * The ButtonImageAlignCenter class provides functionality for aligning an image in the center.
     *
     * @uses ButtonActionStyle
     * @uses ButtonStateClasses
     * @uses ButtonStyle
     *
     * @class ButtonImageAlignCenter
     */
    var ButtonImageAlignCenter = React.createClass({
        mixins: [AlloyEditor.ButtonStyle, AlloyEditor.ButtonStateClasses, AlloyEditor.ButtonActionStyle],

        // Allows validating props being passed to the component.
        propTypes: {
            /**
             * The editor instance where the component is being used.
             *
             * @property {Object} editor
             */
            editor: React.PropTypes.object.isRequired,

            /**
             * The label that should be used for accessibility purposes.
             *
             * @property {String} label
             */
            label: React.PropTypes.string,

            /**
             * The tabIndex of the button in its toolbar current state. A value other than -1
             * means that the button has focus and is the active element.
             *
             * @property {Number} tabIndex
             */
            tabIndex: React.PropTypes.number
        },

        // Lifecycle. Provides static properties to the widget.
        statics: {
            /**
             * The name which will be used as an alias of the button in the configuration.
             *
             * @static
             * @property {String} key
             * @default imageCenter
             */
            key: 'imageCenter'
        },

        /**
         * Lifecycle. Returns the default values of the properties used in the widget.
         *
         * @method getDefaultProps
         * @return {Object} The default properties.
         */
        getDefaultProps: function() {
            return {
                style: {
                    element: 'img',
                    styles: {
                        'display': 'block',
                        'margin': '0 auto',
                        'clear': 'both'
                    }
                }
            };
        },

        /**
         * Lifecycle. Renders the UI of the button.
         *
         * @method render
         * @return {Object} The content which should be rendered.
         */
        render: function() {
            var cssClass = 'ae-button ' + this.getStateClasses();

            return (
                <button aria-label={AlloyEditor.Strings.alignCenter} aria-pressed={cssClass.indexOf('pressed') !== -1} className={cssClass} data-type="button-image-align-left" onClick={this.applyStyle} tabIndex={this.props.tabIndex} title={AlloyEditor.Strings.alignCenter}>
                    <span className="ae-icon-align-center"></span>
                </button>
            );
        }
    });

    AlloyEditor.Buttons[ButtonImageAlignCenter.key] = AlloyEditor.ButtonImageAlignCenter = ButtonImageAlignCenter;
}());

Whether or not to use clear: both on the style is difficult to say, but from a development perspective this should be an easy implementation. It's a little finnicky in one situation: if you don't use clear:both, the image is centered, there's something to it's left or right, and then you bump the image down a few lines so there's no longer something on it's left or right side. I just threw this together in a couple minutes so I won't make a PR with it just now, but if someone doesn't integrate it sometime soon I'll probably come back and make one

@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2015

Hey @ejfrancis,

Thanks for your contribution!
I would specify explicitly the margins, like this:

margin-left: auto;
margin-right: auto

Here is another way to center the image horizontally, let's check how it will look like:

'display': 'block',
'margin-left': '50%',
'transform': 'translateX(-50%)',
'-ms-transform': 'translateX(-50%)'

(The -ms prefix is needed for IE9)

At least in my tests I got better results. For example, open the default example page and try to center the image there. On the first screenshot below it is shown how does it look like with transform:
screen shot 2015-10-06 at 20 38 45

Here is how does it look like with margin-left/margin-right:
screen shot 2015-10-06 at 20 41 59

Both screenshots are without clear: both. If we don't use it, I think what transform does makes more sense. However, if we use it, then at least in this example there is no difference. Could you please test the transform too in your case and tell us if that works better? Then we will decide which one to merge.

Thanks again!
Iliyan

@ejfrancis

This comment has been minimized.

Copy link

commented Oct 6, 2015

@ipeychev yes I like that transform solution better for true centering. I do think both approaches could be desirable in different situations, one for centering relative to the container (transform) and one for centering relative to surrounding content (auto margin), although I imagine true centering with transform would be the more popular choice if you had to pick only one. I'm not sure what type of icon would be used to differentiate the two. I'll give transform a try later today

@ipeychev ipeychev closed this in c10ff83 Oct 7, 2015

ipeychev added a commit that referenced this issue Oct 7, 2015

ipeychev added a commit that referenced this issue Oct 7, 2015

ipeychev added a commit that referenced this issue Oct 7, 2015

@ipeychev ipeychev added this to the 0.6 milestone Oct 7, 2015

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.