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

User forms #157

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

User forms #157

wants to merge 30 commits into from

Conversation

netaustin
Copy link
Contributor

User-facing forms to include in themes, etc.

$classes[] = 'fm-label-inline';
}
if ( $this->label_after_element ) {
$classes[] = 'fm-label-after';
}
// @TODO add inline and block to .label in stylesheet. Test this all.
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this todo has been handled right?

@alexisbellido
Copy link
Contributor

I've tested the user-form branch locally with Selenium IDE and the forms I implemented for European Voice and all of them work fine: user registration, subscription payment, subscription renewal, profile form, and invoice payment.

@alexisbellido
Copy link
Contributor

I've filed an issue slightly related to this: #158

@netaustin
Copy link
Contributor Author

I'm going to hold merging this until tests for Fieldmanager_File are written and FM Demos has an equivalent PR to demo a user-facing form.

type="file"
name="<?php echo $this->get_form_name(); ?>"
id="<?php echo $this->get_element_id(); ?>"
<?php echo $this->get_element_attributes(); ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these templates need escaping?

@nschoenholtz
Copy link
Contributor

Not sure I'm in the right mindset to stamp this officially (too many consecutive pseudoephedrine days), but it does look good to me overall and I'm psyched to play with it (and the tests).

@mboynes
Copy link
Contributor

mboynes commented Aug 31, 2014

AssertTag() is now deprecated. These tests all need to be replaced with, I guess, string matches. Doesn't look like phpunit replaced it with anything.

…ss_class

fix missing fm-element class for textfields
@alexisbellido
Copy link
Contributor

There's a function called fm_renumber in fieldmanager.js that takes care of assigning the correct name and id attributes to HTML elements in a Fieldmanager_Group. See https://github.com/alleyinteractive/wordpress-fieldmanager/blob/user-forms/js/fieldmanager.js#L76

That functions looks for a class .fm-element for each element but the template for textfield is not adding it automatically on this branch, see https://github.com/alleyinteractive/wordpress-fieldmanager/blob/user-forms/templates/textfield.php#L2

This causes information in textfields in a group not to be saved correctly.

A workaround for this could be adding the field_class property to Fieldmanager_Textfield with this value: "fm-element text".

This was fixed on #209

@mboynes mboynes modified the milestone: 1.0 Beta Dec 20, 2014
@mboynes mboynes modified the milestones: later, 1.1.0 Feb 22, 2016
@mboynes mboynes modified the milestones: 1.2.0, 1.3.0 Oct 8, 2017
@mboynes
Copy link
Contributor

mboynes commented Oct 10, 2017

This should not delete Fieldmanager_Context_Page, as others may be using that. We will deprecate it elsewhere first.

@mboynes mboynes mentioned this pull request Oct 10, 2017
8 tasks
@montchr
Copy link
Contributor

montchr commented May 3, 2018

I'm finally checking this out since Fieldmanager_Context_Page has been deprecated.

I've replaced all calls to add_page_form() in my project with add_form() and everything seems to work functionality-wise.

However, styles for the rendered form are a bit messed up:

image 2018-05-03 at 14 32 00

Here's what it should look like normally:

image 2018-05-03 at 14 33 35

Looks like that's happening because the parent of .fm-group-label-wrapper is .fm-fm-group instead of .fm-group. Not sure exactly why that changed.

Also, could someone resolve the merge conflicts so I can test with the changes from master?

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

Successfully merging this pull request may close these issues.

7 participants