Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Fixes #1369 select-list deprecation #1424

Merged
merged 9 commits into from
May 12, 2018
Merged

Fixes #1369 select-list deprecation #1424

merged 9 commits into from
May 12, 2018

Conversation

MatthewDorner
Copy link
Contributor

Fixes #1369.

  • The new ember-rapid-forms select helper mutates the specified property automatically, while the old one required an action. This is a reason for some of the refactoring.
  • Other refactoring because the new one wouldn't work with a simple array (at least I couldn't get it to), so there's some code to change arrays into key/value object arrays.
  • This PR will fail one of the acceptance tests until a new version of ember-rapid-forms is released which fixes an issue encountered in ember-rapid-forms during this work. This will probably be within the next couple weeks. See: action firing on component initialization expected? piceaTech/ember-rapid-forms#193
  • In one instance, language-select.hbs, I went with a native HTML select instead of the ember-rapid-forms select, because the ember-rapid-forms one was adding Bootstrap CSS and caused the appearance to change. Let me know if you'd prefer it done another way. See imgur.com/a/h0ljX1A.
  • Thanks @ben-muldrow for the initial work on this, @efx for reviewing and @GCorbel for helping us with ember-rapid-forms.

cc @HospitalRun/core-maintainers

@MatthewDorner
Merge pull request #1 from HospitalRun/master
615b8f1 
Update my fork from upstream repo.
@MatthewDorner
Merge pull request #3 from HospitalRun/master
7ea8760 
Merge branch 'master'.
@MatthewDorner
Em select review (#5)
a494665 
* Replaced instances of select-list with form.select

* Remvoed selectedLanguage property from language-select.js

* Fixed failing tests

* Removed accidental environment.js changes

* Improved code for computed properties.

* Improved CSS and templates.

* Further fixes to templates

* Compactd map functions, added newlines to end of files.

* Replaced with plain HTML select to avoid CSS styling.

* Improved element spacing in quantity-conv.

* Removed ember-select-list dependency.

* Fixed problem in language-select.

* Added newline to end of file

* Fixed ESLint errors

* Remove debug code, unnecessary setting of properties.

* Revert debug code.
@MatthewDorner
Target new ember-rapid-forms version.
1543e88 
ember-rapid-forms 1.2.5 was released, now the tests for this PR should all pass.
stukalin
stukalin reviewed May 4, 2018
View reviewed changes
app/invoices/edit/template.hbs Outdated
<div class="form-group">
{{select-list
class='form-control'
{{#em-form model=detail submitButton=false as |form|}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it work well? Because you kinda introduce a form within a form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to work the same as before. One thing is you have to define "model=detail" inside the {{#em-form}}, and the form.select uses that to find its property. So I'm not sure if it can work if I define the {{#em-form}} outside of that {{#each item.details as |detail|}} block, though I can try it.

Copy link

Choose a reason for hiding this comment

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

Perhaps we could use the native HTML select here and wire up the actions ourselves to keep it simpler.

Copy link
Contributor

Choose a reason for hiding this comment

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

alright. then I make the PR to "require changes"

stukalin
stukalin approved these changes May 4, 2018
View reviewed changes
Copy link
Contributor

@stukalin stukalin left a comment

Choose a reason for hiding this comment

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

I say approve for now but please answer my question. Just wonder how it works.

@MatthewDorner
Copy link
Contributor Author

Alright, the form-within-a-form is definitely something I should fix. After more research, it's not valid HTML, and also doesn't work on FireFox. So I will start looking into this and update in couple days.

@MatthewDorner
Copy link
Contributor Author

Fixing that also allowed me to remove the added computed property in invoices/edit controller. Now it should be ready to finish reviewing.

stukalin
stukalin suggested changes May 8, 2018
View reviewed changes
app/templates/components/location-select.hbs Outdated
{{#em-form model=this submitButton=false as |form|}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Now I'm wondering if this doesn't introduce the same issue "form within a form" since we use that in a component which could possibly be used in a form. Could you please doublecheck?

Copy link
Contributor

Choose a reason for hiding this comment

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

And sorry for the PR ping-pong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it is used in a form. This one still works in Firefox unlike the first one, but it is still incorrect. Also quantity-conv.hbs does this, maybe others. Thanks for catching this, I just didn't know that about the HTML form tag. I'll have to think about how to handle this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe there is already a rapid form which contains a component. In that case you should be able to pass the rapid form reference in the component and you're done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will try that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of these are multiple levels of component down from the {{#em-form}}, quantity-conv.hbs for example is inventory/edit/template -> (partial 'inv-purchase') -> quantity-calc -> quantity-conv, so it could end up a little confusing. Of course the alternative is another native HTML. It's up to your preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, it works but I had to also add "model=this" in the individual {{form.select}}'s.

@stukalin
Copy link
Contributor

@MatthewDorner while it's not merged... Would you mind to remove ember-views.did-init-attrs from our deprecation-workflow list as of #1370? It shouldn't throw this deprecation anymore.

@tangollama
Copy link
Member

Thank you @MatthewDorner for the PR!

@tangollama tangollama merged commit cefe508 into HospitalRun:master May 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stukalin stukalin stukalin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace ember-select-list with em-select
3 participants
@MatthewDorner @stukalin @tangollama