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

feat: convert FormAssociated to a constructable function #4115

Merged
merged 6 commits into from
Nov 13, 2020

Conversation

radium-v
Copy link
Collaborator

Description

Converts FormAssociated to be a "constructable" function call that returns a mixin class.

Motivation & context

Allows for easier control when working with mixin classes and FormAssociated.

Issue type checklist

  • Chore: A change that does not impact distributed packages.
  • Bug fix: A change that fixes an issue, link to the issue above.
  • New feature: A change that adds functionality.

Is this a breaking change?

  • This change causes current functionality to break.

Adding or modifying component(s) in @microsoft/fast-components checklist
This modifies all FormAssociated-based components:

  • Button

  • Checkbox

  • Radio

  • Slider

  • Text Area

  • Text Field

  • I have added a new component

  • I have modified an existing component

  • I have updated the definition file

  • I have updated the configuration file

Process & policy checklist

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

@radium-v
Copy link
Collaborator Author

@nicholasrice there are still two tests that are breaking for me which were added recently, can you take a look and see if there's a quick fix?

@radium-v radium-v force-pushed the users/jokreitl/form-associated branch from ca78bf0 to 3143cc7 Compare November 12, 2020 03:03
Copy link
Contributor

@EisenbergEffect EisenbergEffect left a comment

Choose a reason for hiding this comment

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

I like this change. I made one request to move an interface. If we can make that change and get the tests all passing, I think we're good.

@nicholasrice
Copy link
Contributor

I think the FormAssociated test assertion can be changed to expect empty string. At this point I don't recall why that test is there, but if you check the value property of an <input> prior to connection it is empty string, so I think we should match that behavior.

@radium-v radium-v force-pushed the users/jokreitl/form-associated branch from 3143cc7 to ae62012 Compare November 13, 2020 04:52
@radium-v radium-v self-assigned this Nov 13, 2020
Copy link
Contributor

@nicholasrice nicholasrice left a comment

Choose a reason for hiding this comment

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

Thanks @radium-v - this is awesome.

Copy link
Member

@chrisdholt chrisdholt left a comment

Choose a reason for hiding this comment

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

That's a lot of updates :)

Thanks @radium-v!

@radium-v radium-v merged commit da8d54b into master Nov 13, 2020
@radium-v radium-v deleted the users/jokreitl/form-associated branch November 13, 2020 19:38
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.

4 participants