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

DNS: more consistent and realistic handling of DNS events #1661

Merged
merged 1 commit into from
Dec 16, 2015

Conversation

klimeryk
Copy link
Contributor

Changes inspired by a comment by @stephanethomas.
Previously, we optimistically deleted the DNS record. Now, we wait for the response from the API. Same for adding DNS records.
The DNS_*_COMPLETED events are now only fired when there's no error, making reducer implementation more straightforward.

I did not add the DNS_DELETE and DNS_ADD events, since they are not needed right now and might suggest that something is happening (or should be) when they are fired. If someone will need them (for example, when implementing optimistic versions of those actions), it's trivial to add them.

While getting rid of the onComplete callback and keeping everything pure was tempting (as discussed with Stephane), looking at the fluxing (ha-ha, get it?) approach to Redux usage right now in Calypso, I decided it's beyond my abilities at the moment. But I'll keep a close eye on the developments in this area and when I'll educate myself and have some spare cycles I'll get back to this :)

cc:

  • @dzver (although, I'm sure he already felt the disturbance in the Force)
  • @aidvu (sorry it's not a bigger change that you could nitpick easily, like I did 😁)

@klimeryk klimeryk added the [Feature Group] Emails & Domains Features related to email integrations and domain management. label Dec 16, 2015
Previously, we optimistically deleted the DNS record. Now, we wait for
the response from the API. Same for adding DNS records.
The DNS_*_COMPLETED events are now only fired when there's no error,
making reducer implementation more straightforward.
@klimeryk klimeryk force-pushed the fix/missing-domain-events branch from 1b66c43 to 6742f49 Compare December 16, 2015 03:03
@klimeryk klimeryk added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Task labels Dec 16, 2015
@dzver
Copy link
Member

dzver commented Dec 16, 2015

👍

I cannot help but notice how the recent changes of the DNS editor have significantly worsened the UX. It's not supposed to work like it works right now.

  1. When you click add, the button needs to become gray/disabled
  2. When you click delete, it needs to disappear immediately.

@klimeryk
Copy link
Contributor Author

I agree that the DNS actions are not as responsive as they could be. I'll merge this as-is now, and will try to improve this situation in some spare time.

@klimeryk klimeryk removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 16, 2015
klimeryk added a commit that referenced this pull request Dec 16, 2015
DNS: more consistent and realistic handling of DNS events
@klimeryk klimeryk merged commit 9dffb14 into master Dec 16, 2015
@klimeryk klimeryk deleted the fix/missing-domain-events branch December 16, 2015 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature Group] Emails & Domains Features related to email integrations and domain management. [Type] Task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants