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

Add "busy button" and Publish Flow. #3682

Merged
merged 3 commits into from Nov 28, 2017

Conversation

Projects
None yet
5 participants
@mtias
Contributor

mtias commented Nov 27, 2017

image

See #3496.

  • Figure out how to distinguish "publishing..." from "updating...".

cc @jasmussen @karmatosed

@mtias mtias added the Design label Nov 27, 2017

@mtias mtias changed the title from Add/busy button pubish flow to Add "busy button" and publish flow. Nov 27, 2017

@mtias mtias changed the title from Add "busy button" and publish flow. to Add "busy button" and Publish Flow. Nov 27, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 27, 2017

Codecov Report

Merging #3682 into master will increase coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3682      +/-   ##
==========================================
+ Coverage    37.3%   37.46%   +0.15%     
==========================================
  Files         277      277              
  Lines        6690     6711      +21     
  Branches     1214     1221       +7     
==========================================
+ Hits         2496     2514      +18     
- Misses       3536     3539       +3     
  Partials      658      658
Impacted Files Coverage Δ
components/button/index.js 90.9% <ø> (ø) ⬆️
...tor/components/post-publish-with-dropdown/index.js 0% <ø> (ø) ⬆️
editor/effects.js 58.87% <100%> (+0.8%) ⬆️
editor/selectors.js 93.43% <100%> (+0.31%) ⬆️
editor/components/post-publish-button/label.js 87.5% <100%> (+4.16%) ⬆️
editor/components/post-featured-image/index.js 35.29% <0%> (+13.07%) ⬆️

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 1db24fc...ec4dc7c. Read the comment docs.

codecov bot commented Nov 27, 2017

Codecov Report

Merging #3682 into master will increase coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3682      +/-   ##
==========================================
+ Coverage    37.3%   37.46%   +0.15%     
==========================================
  Files         277      277              
  Lines        6690     6711      +21     
  Branches     1214     1221       +7     
==========================================
+ Hits         2496     2514      +18     
- Misses       3536     3539       +3     
  Partials      658      658
Impacted Files Coverage Δ
components/button/index.js 90.9% <ø> (ø) ⬆️
...tor/components/post-publish-with-dropdown/index.js 0% <ø> (ø) ⬆️
editor/effects.js 58.87% <100%> (+0.8%) ⬆️
editor/selectors.js 93.43% <100%> (+0.31%) ⬆️
editor/components/post-publish-button/label.js 87.5% <100%> (+4.16%) ⬆️
editor/components/post-featured-image/index.js 35.29% <0%> (+13.07%) ⬆️

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 1db24fc...ec4dc7c. Read the comment docs.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Nov 27, 2017

Member

Pushed ec4dc7c which implements the isPublishingPost selector. One challenge with this implementation was knowing the transaction ID associated with the in-progress save. Since we previously generated a unique ID, we couldn't necessarily know this without otherwise saving a reference to it. With my changes, this was updated to a static value that can be referenced directly by the selector. I don't think this should be an issue, both because we don't allow simultaneous save requests to occur, but even if we did, from what I can tell redux-optimist will still clear itself out when the doubled-up transaction is committed. Another option could be to check whether a transaction has already begun when saving the post, and commit it prior to beginning a new one.

cc @youknowriad

Member

aduth commented Nov 27, 2017

Pushed ec4dc7c which implements the isPublishingPost selector. One challenge with this implementation was knowing the transaction ID associated with the in-progress save. Since we previously generated a unique ID, we couldn't necessarily know this without otherwise saving a reference to it. With my changes, this was updated to a static value that can be referenced directly by the selector. I don't think this should be an issue, both because we don't allow simultaneous save requests to occur, but even if we did, from what I can tell redux-optimist will still clear itself out when the doubled-up transaction is committed. Another option could be to check whether a transaction has already begun when saving the post, and commit it prior to beginning a new one.

cc @youknowriad

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 28, 2017

Contributor

Changing the label of the button while saving introduces a weird flickering (the button size changes and reverts quickly) while auto-saving. Do you we could avoid changing the label and just add an is-busy style?

Contributor

youknowriad commented Nov 28, 2017

Changing the label of the button while saving introduces a weird flickering (the button size changes and reverts quickly) while auto-saving. Do you we could avoid changing the label and just add an is-busy style?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Nov 28, 2017

Contributor

Changing the label of the button while saving introduces a weird flickering (the button size changes and reverts quickly) while auto-saving. Do you we could avoid changing the label and just add an is-busy style?

This is a good point, but it may be worth it. Two options I can think of:

  1. We add a min-width to the button, so the label can increase in width, without a judder. Probably not scalable with translations.
  2. We add a transition to the button width, and see if that makes it feel less jarring — it could potentially be a non-issue.

What do you think?

Contributor

jasmussen commented Nov 28, 2017

Changing the label of the button while saving introduces a weird flickering (the button size changes and reverts quickly) while auto-saving. Do you we could avoid changing the label and just add an is-busy style?

This is a good point, but it may be worth it. Two options I can think of:

  1. We add a min-width to the button, so the label can increase in width, without a judder. Probably not scalable with translations.
  2. We add a transition to the button width, and see if that makes it feel less jarring — it could potentially be a non-issue.

What do you think?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 28, 2017

Contributor

Fixing the width would have been great but it's not possible with translations. I can live with it, but I think we should avoid the label changing at least for "auto-saves" because it's not an "explicit" click and it's a duplicated information with the Saving indicator.

Contributor

youknowriad commented Nov 28, 2017

Fixing the width would have been great but it's not possible with translations. I can live with it, but I think we should avoid the label changing at least for "auto-saves" because it's not an "explicit" click and it's a duplicated information with the Saving indicator.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Nov 28, 2017

Contributor

Agreed, we have the save status indicator for the autosave, the button itself should probably be only for publishing, updating, and scheduling, as well as "working" versions of those.

Riad in a little noodling on this branch I can't get the CSS transition to have any effect on the width of this — is there something I'm missing or is it just not possible?

Contributor

jasmussen commented Nov 28, 2017

Agreed, we have the save status indicator for the autosave, the button itself should probably be only for publishing, updating, and scheduling, as well as "working" versions of those.

Riad in a little noodling on this branch I can't get the CSS transition to have any effect on the width of this — is there something I'm missing or is it just not possible?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 28, 2017

Contributor

Riad in a little noodling on this branch I can't get the CSS transition to have any effect on the width of this — is there something I'm missing or is it just not possible?

I think it's because the width property do not change, it's the content that changes

Contributor

youknowriad commented Nov 28, 2017

Riad in a little noodling on this branch I can't get the CSS transition to have any effect on the width of this — is there something I'm missing or is it just not possible?

I think it's because the width property do not change, it's the content that changes

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Nov 28, 2017

Contributor

I think you're probably right. I tried some tricks, but couldn't get it to work.

Contributor

jasmussen commented Nov 28, 2017

I think you're probably right. I tried some tricks, but couldn't get it to work.

@jasmussen jasmussen referenced this pull request Nov 28, 2017

Closed

Improve Publishing Flow #3496

@StaggerLeee

This comment has been minimized.

Show comment
Hide comment
@StaggerLeee

StaggerLeee Nov 28, 2017

What is that "gradient" on your button ?

StaggerLeee commented Nov 28, 2017

What is that "gradient" on your button ?

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Nov 28, 2017

Contributor

It's a transition that signals something is happening and the button is not just disabled.

Contributor

mtias commented Nov 28, 2017

It's a transition that signals something is happening and the button is not just disabled.

@mtias mtias merged commit 8d3e230 into master Nov 28, 2017

3 checks passed

codecov/project 37.46% (+0.15%) compared to 1db24fc
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mtias mtias deleted the add/busy-button-pubish-flow branch Nov 28, 2017

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