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.

Sign up for GitHub

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

Jump to bottom

Help: Rethink the way we render the HelpContactForm #1116

Merged
merged 2 commits into from
Dec 4, 2015
Merged

Help: Rethink the way we render the HelpContactForm #1116

merged 2 commits into from
Dec 4, 2015

Conversation

omarjackman
Copy link
Contributor

As previously discussed with @dmsnell I modified the logic that sets up the rendering for the HelpContactForm. Instead of using multiple functions that return HelpContactForm react classes I now have a single point where the React class is used and I'm using functions to provide the appropriate properties

@omarjackman omarjackman self-assigned this Dec 1, 2015
@omarjackman omarjackman added [Status] In Progress [Feature Group] Support All things related to WordPress.com customer support. labels Dec 1, 2015
@omarjackman omarjackman changed the title Rethink the way we render the HelpContactForm Help: Rethink the way we render the HelpContactForm Dec 1, 2015
@omarjackman omarjackman added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Dec 1, 2015
@omarjackman
Copy link
Contributor Author

cc @dmsnell

@omarjackman omarjackman mentioned this pull request Dec 1, 2015
Merged
dmsnell
dmsnell reviewed Dec 2, 2015
View reviewed changes
client/me/help/help-contact/index.jsx
}

return this.getKayakoTicketForm();
// Hide the olark widget in the bottom right corner.
olarkActions.hideBox();
Copy link
Member

Choose a reason for hiding this comment

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

This is just a DOM manipulation for a global element, right? Just checking that we aren't attempting to change the component's state mid-render.

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 is just a DOM manipulation for a global element, right? Just checking that we aren't attempting to change the component's state mid-render.

Thats correct.

@dmsnell
Copy link
Member

dmsnell commented Dec 2, 2015

Looks good @omarjackman. The gains here aren't as big as I had hoped. On the other hand, I can see how the early-return logic in getView() is making this considerably easier. One final observation is that several of the props in the Kayako and Chat forms are the same, meaning we might be able to eliminate those two get...Properties calls with something of a hybrid...

@omarjackman omarjackman added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 3, 2015
@omarjackman omarjackman force-pushed the update/help-contact-form-props branch from 4ba4e8a to c808a47 Compare December 3, 2015 00:28
@omarjackman
Use a single HelpContactForm component where the props are conditiona…
fc09183 
…lized rather than duplicating its use in multiple functions.
@omarjackman omarjackman force-pushed the update/help-contact-form-props branch from c808a47 to fc09183 Compare December 3, 2015 00:30
@omarjackman omarjackman added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Dec 3, 2015
@omarjackman
Copy link
Contributor Author

@dmsnell I did a rebase which caused the logic here to get a bit more complicated. Take a look at the new changes.

@omarjackman omarjackman force-pushed the update/help-contact-form-props branch 2 times, most recently from bf6ad59 to 59ca9df Compare December 3, 2015 01:43
@dmsnell
Copy link
Member

dmsnell commented Dec 3, 2015

Looks fine. Go for it!

@dmsnell dmsnell added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 3, 2015
@omarjackman
Eliminate the different function calls for generating contact form pr…
73cd30c 
…operties so that we can avoid duplication.

We'll now use a single object to hold the properties for the different variations of the help contact form.
@omarjackman omarjackman force-pushed the update/help-contact-form-props branch from 59ca9df to 73cd30c Compare December 4, 2015 00:45
omarjackman added a commit that referenced this pull request Dec 4, 2015
@omarjackman
Merge pull request #1116 from Automattic/update/help-contact-form-props
38809a0 
Help: Rethink the way we render the HelpContactForm
@omarjackman omarjackman merged commit 38809a0 into master Dec 4, 2015
@omarjackman omarjackman deleted the update/help-contact-form-props branch December 4, 2015 01:00
@ajesse11x ajesse11x mentioned this pull request Nov 4, 2022
Open
@awaisab172 awaisab172 mentioned this pull request Nov 5, 2022
Open
@ajesse11x ajesse11x mentioned this pull request Dec 25, 2022
Open
@awaisab172 awaisab172 mentioned this pull request Dec 26, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@omarjackman omarjackman

Labels
[Feature Group] Support All things related to WordPress.com customer support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@omarjackman @dmsnell @scruffian