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

Upgrade to Ember 3.4 (x-select v4.0) #224

Merged
merged 16 commits into from
Oct 22, 2018
Merged

Upgrade to Ember 3.4 (x-select v4.0) #224

merged 16 commits into from
Oct 22, 2018

Conversation

Robdel12
Copy link
Collaborator

@Robdel12 Robdel12 commented Oct 9, 2018

What is this?

Buckle in, because this is a biggin'.

What started as a simple deprecation warning fix (#223) turned into a full blown upgrade. I needed to remove sendAction from the component, which would be a breaking change and require a major version bump. Doing a major version bump is a perfect spot to get other breaking changes you need done, so that's what I did here.

User facing

Breaking changes

<XSelect> 4.0 has the following breaking changes:

  • Remove jQuery from addon (no longer passes jQuery events to action handlers or uses jQuery)
  • Change test helper to use @bigtest/interactor
  • Remove sendAction. on-change is now the default action fired (always was under the hood).
  • Remove the actionprop ({{#x-select action="myAction"}})

Non-breaking changes

There are a few changes that are user facing but not breaking changes:

  • Update the README to use the new angle bracket syntax.
  • Update the README to explain the new test helper
  • Added a new migration guide for migrating to 4.x
  • tabindex is now default to null since <select>s are interactive elements be default. Applying tabindex="0" is redundant.
  • Added is-selected class to the selected option

Non-user facing changes

The non-user facing changes / upgrades to the addon are:

  • Upgrade Ember-CLI to 3.4
  • Upgrade the test helpers & suite to the new ember testing tools. (For async / await)
  • Rewrite a lot of the test suite to make use of integration tests. Some tests were not really an acceptance test (by reaching into the controller and manipulating that directly).
  • Rewrite the test suite to use the new test helper (interactor). That's right, we're dog fooding it.
  • Removed all jQuery from tests
  • Removed the fs-branding addon. It needed to be upgraded and it's not useful being abstracted (since no other addons are using it)
  • Ran prettier through the codebase to normalize all code style.

What's the plan after merge?

I'd like to release 4.0.0-beta.x for the community to try out. If no issues are found in say 3-4 weeks (or longer. Or sooner 🤷‍♂️), we'll release 4.0.0 proper. 🎉

TODOs

  • Fill out PR description properly
  • Update Changelog
  • Document all breaking changes
  • Update README
  • Update dummy app documentation
  • Create migration guide
  • Add docs for how to use the interactor test helper

This needs a whole lot of work. We need to remove jquery from
everything.
README.md Outdated
{{/x-select}}
<XSelect @value={{bob}} action={{action "selectPerson"}} as |xs|>
<xs.option @value={{fred}}}}Fred Flintstone</xs.option>
<xs.option @value={{bob}}}}Bob Newhart</xs.option>

Choose a reason for hiding this comment

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

Looks like there are some left over } instead of > in this file

@Robdel12 Robdel12 force-pushed the rd/upgrade-ember branch 5 times, most recently from 7940e5c to 13bbf5f Compare October 12, 2018 15:13
README.md Outdated
`<XSelect>` 4.0 ships with an entirely new test helper that goes
beyond just allowing you to select an option. It allows you to
interact with your `<select>` element in all different ways. For
example in tests if you need to assert your first options `disabled`
Copy link

@wwilsman wwilsman Oct 12, 2018

Choose a reason for hiding this comment

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

For example, (comma)

assert that your first option is

README.md Outdated

``` javascript
import { select } from "yourappname/tests/helpers/x-select";
// you can name the import whatever you want
import XSelect from "yourappname/tests/helpers/x-select";

Choose a reason for hiding this comment

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

Just for the sake of the rest of the examples here, I think it should be XSelectInteractor

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hahah that's funny, I was trying to hand wave over the fact that we're using an interactor and make it just about the helper. But I agree, there's no escaping it really.

// ...

test('Selecting an option', async (assert) => {
await xselect.select('Fred Flintstone');
Copy link

@wwilsman wwilsman Oct 12, 2018

Choose a reason for hiding this comment

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

I was going to mention leaving a quick comment that says something like "the interactor resolves after interacting with the select, but not after any change events are handled" which is why we use when for the following assertion.

But then that made me think that people will pretty much require when whenever they use this test helper. Or we can override the default select method to resolve after it is selected. It would still throw an error when it cannot select or when the option does not get selected, and it would mean people can expect as usual after awaiting on select(option).

@Robdel12 Robdel12 requested a review from cowboyd October 15, 2018 22:50
@Robdel12 Robdel12 changed the title [WIP] Upgrade to Ember 3.4 (x-select v4.0) Upgrade to Ember 3.4 (x-select v4.0) Oct 15, 2018
@cowboyd
Copy link
Collaborator

cowboyd commented Oct 15, 2018

Nice work @Robdel12 🎉

I have one suggestion, and one question. Both are related to the fact that we're introducing breaking changes.

  1. With angle brackets, I believe that the @ properties should follow javascript naming conventions since they are javascript variables. Rather than html properties like it was before... So @onSelect instead of @on-select Maybe we can deprecate the @on-select Property and then remove it in v5 or something.

  2. Should we consider the case of having the select each over the options itself so that we can get rid of the need for all the registration poo?

<XSelect @over={items} as |Option item|>
  <Option>{item.name}</Option>
</XSelect>

Or should that be a v5 thing? I'm just mentioning it now since we're breaking things.

@Robdel12
Copy link
Collaborator Author

🤔

  1. Hmm, I'm totally not opposed to that. I think we can do that in a follow on PR after this one for v4?
  2. That one feels more like a v5 thing to me. It'd be interesting to explore that change looks like for existing users too

@Robdel12 Robdel12 merged commit a7a30d8 into master Oct 22, 2018
@Robdel12 Robdel12 deleted the rd/upgrade-ember branch November 22, 2018 07:41
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.

3 participants