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

Feature/476 show progress indicator #527

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Feature/476 show progress indicator #527

wants to merge 5 commits into from

Conversation

davidsword
Copy link
Contributor

@davidsword davidsword commented Sep 21, 2018

Task:

add progress indicators on to a liveblogs entries Publish New, Publish Update, and Delete buttons. See #476

Reason:

If on a slower network/server, when clicking the 'Publish New Entry', 'Publish Update Entry', or 'Delete' entry buttons, the user receives no feedback, no indication that their click is actually doing anything. This can be confusing or lead to repetitive clicks or miss-clicks.

Solution:

Add states during these 'transactions' using pre-existing Redux actions/reducers to monitor this transaction progress. Components can conditionally check and show progress indicators by changing text or classNames as needed.

Execution: See commits, or tldr:

  • 'Delete' button: after a user clicks, add a true value to the <EntryContainer> components isDeleting state. When isDeleting, a CSS animation class gets added to the delete button and the text changes. This is all self contained in the component instead of using Store, as once the Entry is deleted, the component will be removed from state, unmounting, and no further state change is required.

  • 'Publish Update' button: when editing a entry, theres a pre-existing isEditing state stored in store > user > entries > (entry id). I added another item to the object, isPublishing. The value is set to true when the Redux action UPDATE_ENTRY is fired. On the UPDATE_ENTRY_SUCCESS action, isPublishing state to is set back to false. When isPublishing is true, a CSS class is added to the button and its text changes.

  • 'Publish New Update' button: almost the exact same as above, except I used isPublishingNew and store it in store > api. It sets to true between the CREATE_ENTRY and CREATE_ENTRY_SUCCESS actions.

Screenshot:

demo2

When a user on a slower network/server clicked delete, there was no 
visual feedback that their action was processing.

Adding this state allows ternary checks within the component to change 
the buttons text & add a 'active' CSS animation class.

This gives the user a indicator of their delete action is being 
preformed.


When a user on a slower network/server clicked Publish New Entry, there 
was no visual feedback that their action was processing.

Adding this state by using pre-existing Redux actions, we can know when 
the Publishing transaction is occurring, allowing ternary checks within 
the components, which gives the availability to change the buttons text 
and classes.
When a user on a slower network/server clicked Publish Update, there was 
no visual feedback that their action was processing.

Adding this state to the entries by using pre-existing Redux actions, we 
can know when the Publishing transaction is occurring, allowing 
conditional checks within the components. This gives the option to 
change the buttons text and classes, which can indicate progress.
When a user on a slower network/server clicked Publish Update, there was 
no visual feedback that their action was processing.

By using the isPublishingNew state for the main 'add new entry' form, 
and using the isPublish state while editing the contents of individual 
entries, the component can now change the button text and buttons css 
class to indicate the action is in progress.
This function was used to close the editor immediately after clicking 
'Publish Update'. When a user on a slower network/server clicked this, 
the editor disappeared instantly, however it would take several seconds 
for the entries text to actually update - there was no visual feedback 
that their action was processing.

By not using this function the editor does not instantly get removed. 
Not being removed right away allows the editor component to check if the 
`store > user > entries` entry has a true isPublishing state, adding 
necessary indicators of progress.

The editor is already set to close by changing the isEditing state to 
false, on the UPDATE_ENTRY_SUCCESS Redux action in the user.js reducer.
@@ -87,7 +87,7 @@ class EditorContainer extends Component {
}

publish() {
const { updateEntry, entry, entryEditClose, createEntry, isEditing } = this.props;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we removing entryEditClose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit explains it: 0ef8038

This function was used to close the editor immediately after clicking 'Publish Update'. When a user on a slower network/server clicked this, the editor disappeared instantly, however it would take several seconds for the entries text to actually update - there was no visual feedback that their action was processing.

By not using this function the editor does not instantly get removed. Not being removed right away allows the editor component to check if the store > user > entries entry has a true isPublishing state, adding necessary indicators of progress.

The editor is already set to close by changing the isEditing state to false, on the UPDATE_ENTRY_SUCCESS Redux action in the user.js reducer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes @sboisvert, I see the problem now, I had that in the wrong commit. My mistake.

Copy link
Contributor

@sboisvert sboisvert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this commit answers the question from the previous commit. Perhaps it was incidental but having that line be part of this commit would probably be more logical. Just a thought for the future :)

Copy link
Contributor

@sboisvert sboisvert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Let's merge this for the next version!

@sboisvert
Copy link
Contributor

I feel like the build failures here are unrelated.

@davidsword
Copy link
Contributor Author

@sboisvert the failing I believe is: #529

@GaryJones
Copy link
Contributor

Looks like there's some stylelint items to address, and a merge conflict.

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

Successfully merging this pull request may close these issues.

4 participants