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

Button: Add "Start on a new line" option (#1473) #1512

Merged
merged 1 commit into from Oct 6, 2017

Conversation

Projects
None yet
4 participants
@truongwp
Contributor

truongwp commented Jun 27, 2017

There is no way to clear the float when using an align left or right button, the next text button always behinds the button. You can read more here: #1473

@truongwp truongwp closed this Jun 27, 2017

@truongwp

This comment has been minimized.

Show comment
Hide comment
@truongwp

truongwp Jun 27, 2017

Contributor

I will pull and rebase to avoid marge commit.

Contributor

truongwp commented Jun 27, 2017

I will pull and rebase to avoid marge commit.

@truongwp truongwp reopened this Jun 27, 2017

@gziolo

It looks like it addresses #1473. Can you rebase with master and provide testing instructions? I'm happy to help to land it into master.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 23, 2017

Codecov Report

Merging #1512 into master will decrease coverage by 0.09%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1512     +/-   ##
=========================================
- Coverage   33.77%   33.67%   -0.1%     
=========================================
  Files         191      190      -1     
  Lines        5691     5686      -5     
  Branches      996      993      -3     
=========================================
- Hits         1922     1915      -7     
- Misses       3189     3191      +2     
  Partials      580      580
Impacted Files Coverage Δ
blocks/library/button/index.js 19.04% <0%> (-7.62%) ⬇️
editor/selectors.js 94.73% <0%> (-2.02%) ⬇️
blocks/editable/index.js 10.31% <0%> (-0.19%) ⬇️
blocks/library/table/index.js 41.66% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
editor/store-persist.js 100% <0%> (ø) ⬆️
blocks/library/code/index.js 57.14% <0%> (ø) ⬆️
...ditor/modes/visual-editor/invalid-block-warning.js 0% <0%> (ø) ⬆️
blocks/library/shortcode/index.js 50% <0%> (ø) ⬆️
blocks/library/pullquote/index.js 33.33% <0%> (ø) ⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff1ef2b...07e5a06. Read the comment docs.

codecov bot commented Sep 23, 2017

Codecov Report

Merging #1512 into master will decrease coverage by 0.09%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1512     +/-   ##
=========================================
- Coverage   33.77%   33.67%   -0.1%     
=========================================
  Files         191      190      -1     
  Lines        5691     5686      -5     
  Branches      996      993      -3     
=========================================
- Hits         1922     1915      -7     
- Misses       3189     3191      +2     
  Partials      580      580
Impacted Files Coverage Δ
blocks/library/button/index.js 19.04% <0%> (-7.62%) ⬇️
editor/selectors.js 94.73% <0%> (-2.02%) ⬇️
blocks/editable/index.js 10.31% <0%> (-0.19%) ⬇️
blocks/library/table/index.js 41.66% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
editor/store-persist.js 100% <0%> (ø) ⬆️
blocks/library/code/index.js 57.14% <0%> (ø) ⬆️
...ditor/modes/visual-editor/invalid-block-warning.js 0% <0%> (ø) ⬆️
blocks/library/shortcode/index.js 50% <0%> (ø) ⬆️
blocks/library/pullquote/index.js 33.33% <0%> (ø) ⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff1ef2b...07e5a06. Read the comment docs.

@truongwp

This comment has been minimized.

Show comment
Hide comment
@truongwp

truongwp Sep 23, 2017

Contributor

@gziolo I rebased with master. Below is the problem and the test case:

I want to align my button to the right, then next line is a paragraph, like this:

selection_001

But if I align my button right, it floats and the second paragraph goes beside the button:

selection_002

With the "Start on a new line" option, the button is always on a line, even when floats left, right. We can discuss about "Start on a new line" phrase. I can't find a better phrase.

Contributor

truongwp commented Sep 23, 2017

@gziolo I rebased with master. Below is the problem and the test case:

I want to align my button to the right, then next line is a paragraph, like this:

selection_001

But if I align my button right, it floats and the second paragraph goes beside the button:

selection_002

With the "Start on a new line" option, the button is always on a line, even when floats left, right. We can discuss about "Start on a new line" phrase. I can't find a better phrase.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 23, 2017

Contributor

related #1473

Contributor

youknowriad commented Sep 23, 2017

related #1473

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Sep 25, 2017

Member

With the "Start on a new line" option, the button is always on a line, even when floats left, right. We can discuss about "Start on a new line" phrase. I can't find a better phrase.

@karmatosed or @jasmussen any suggestions on the name this option?

Member

gziolo commented Sep 25, 2017

With the "Start on a new line" option, the button is always on a line, even when floats left, right. We can discuss about "Start on a new line" phrase. I can't find a better phrase.

@karmatosed or @jasmussen any suggestions on the name this option?

@truongwp

This comment has been minimized.

Show comment
Hide comment
@truongwp

truongwp Sep 25, 2017

Contributor

What about this phrase: "Stand on a line"

Contributor

truongwp commented Sep 25, 2017

What about this phrase: "Stand on a line"

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 25, 2017

Contributor

What about this phrase: "Stand on a line"

I like that.

Contributor

jasmussen commented Sep 25, 2017

What about this phrase: "Stand on a line"

I like that.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 25, 2017

Contributor

Just a note that this is probably something we can handle as an "extension" to blocks, the same way we're thinking of refactoring the custom className and anchor properties. See here for more details #2474 (comment)

Contributor

youknowriad commented Sep 25, 2017

Just a note that this is probably something we can handle as an "extension" to blocks, the same way we're thinking of refactoring the custom className and anchor properties. See here for more details #2474 (comment)

@gziolo

gziolo approved these changes Sep 25, 2017

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Sep 25, 2017

Member

Code looks good, it works as advertised. It only needs an option name to be updated as discussed above and it should be good to merge. Thank you very much for working on that one.

Member

gziolo commented Sep 25, 2017

Code looks good, it works as advertised. It only needs an option name to be updated as discussed above and it should be good to merge. Thank you very much for working on that one.

@truongwp

This comment has been minimized.

Show comment
Hide comment
@truongwp

truongwp Sep 25, 2017

Contributor

@gziolo Thank you. Do I need to update the commit to change option name?

Contributor

truongwp commented Sep 25, 2017

@gziolo Thank you. Do I need to update the commit to change option name?

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Sep 25, 2017

Member

That would be the best. You can also add another commit and we can squash before merge.

Member

gziolo commented Sep 25, 2017

That would be the best. You can also add another commit and we can squash before merge.

@truongwp

This comment has been minimized.

Show comment
Hide comment
@truongwp

truongwp Sep 25, 2017

Contributor

I updated the commit with option text is "Stand on a line"

Contributor

truongwp commented Sep 25, 2017

I updated the commit with option text is "Stand on a line"

@truongwp

This comment has been minimized.

Show comment
Hide comment
@truongwp

truongwp Sep 26, 2017

Contributor

@gziolo I added spaces around clearControl. Just wait CI to check code :D

Contributor

truongwp commented Sep 26, 2017

@gziolo I added spaces around clearControl. Just wait CI to check code :D

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Sep 26, 2017

Member

Travis CI seems to be unhappy with this change which is surprising and completely unexpected. I'm trying to find out what has happened on Slack's #core-editor channel.

Member

gziolo commented Sep 26, 2017

Travis CI seems to be unhappy with this change which is surprising and completely unexpected. I'm trying to find out what has happened on Slack's #core-editor channel.

@truongwp

This comment has been minimized.

Show comment
Hide comment
@truongwp

truongwp Sep 26, 2017

Contributor

Sometimes I meet this error. I think it isn't coding error.

Contributor

truongwp commented Sep 26, 2017

Sometimes I meet this error. I think it isn't coding error.

@truongwp

This comment has been minimized.

Show comment
Hide comment
@truongwp

truongwp Sep 29, 2017

Contributor

I moved "Stand on a line" options to below of block description.

Contributor

truongwp commented Sep 29, 2017

I moved "Stand on a line" options to below of block description.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Oct 2, 2017

Member

This is how it looks like at the moment:

screen shot 2017-10-02 at 11 38 07

I moved "Stand on a line" options to below of block description.

@jasmussen I'm not familiar with the patterns here, can you confirm it fits well?

Another question is if we should remove max-width: 370px when Stand on a line is enabled? It seems like it should take all space in such case, similar to when it's center aligned.

Member

gziolo commented Oct 2, 2017

This is how it looks like at the moment:

screen shot 2017-10-02 at 11 38 07

I moved "Stand on a line" options to below of block description.

@jasmussen I'm not familiar with the patterns here, can you confirm it fits well?

Another question is if we should remove max-width: 370px when Stand on a line is enabled? It seems like it should take all space in such case, similar to when it's center aligned.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 2, 2017

Contributor

Another question is if we should remove max-width: 370px when Stand on a line is enabled? It seems like it should take all space in such case, similar to when it's center aligned.

I think we had the 370px value back when we didn't have image resizing, and perhaps the button just inherited it. It seems like if the button width can expand the block border itself, then that would be superior to having a fixed width. What do you think, @mtias?

By the way we should reconsider that link thing on the button. I know why we did it initially, but two toolbars is too much for that block. We should have a link button in the toolbar that just applies at the block level to the block.

Contributor

jasmussen commented Oct 2, 2017

Another question is if we should remove max-width: 370px when Stand on a line is enabled? It seems like it should take all space in such case, similar to when it's center aligned.

I think we had the 370px value back when we didn't have image resizing, and perhaps the button just inherited it. It seems like if the button width can expand the block border itself, then that would be superior to having a fixed width. What do you think, @mtias?

By the way we should reconsider that link thing on the button. I know why we did it initially, but two toolbars is too much for that block. We should have a link button in the toolbar that just applies at the block level to the block.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Oct 6, 2017

Member

Okey, it looks like this width issue is unrelated to this change. So let's fix it in another PR if we agree that it makes sense. Merging this one as it is. @truongwp thanks for your contribution 👍

Member

gziolo commented Oct 6, 2017

Okey, it looks like this width issue is unrelated to this change. So let's fix it in another PR if we agree that it makes sense. Merging this one as it is. @truongwp thanks for your contribution 👍

@gziolo gziolo merged commit 1400611 into WordPress:master Oct 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth referenced this pull request May 3, 2018

Closed

Remove wrap text setting in button block #6567

3 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment