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

Update to ember-cli 3.28 #145

Closed
wants to merge 33 commits into from
Closed

Update to ember-cli 3.28 #145

wants to merge 33 commits into from

Conversation

Gaurav0
Copy link

@Gaurav0 Gaurav0 commented Nov 22, 2021

No description provided.

Copy link
Contributor

@Kerrick Kerrick left a comment

Choose a reason for hiding this comment

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

Partial review

module.exports = {
extends: 'recommended',
rules: {
'no-invalid-interactive': false,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's a mistake to turn this one off.

Copy link
Author

@Gaurav0 Gaurav0 Nov 24, 2021

Choose a reason for hiding this comment

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

For accessibility, absolutely. For backward compatibility, however, no. We will have to fix accessibility when we are ready to break compatibility. Moving actions to dom nodes that are interactive will definitely break things.

.travis.yml Outdated
@@ -1,10 +1,11 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this file if we're switching to GitHub Actions?

Copy link
Author

Choose a reason for hiding this comment

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

It's not going to run given travisci.org is dead. 👍

Copy link
Author

Choose a reason for hiding this comment

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

For some reason github is still expecting a travis run. This may be fixable through settings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good call. I just deleted it from the required status checks.

@@ -1,5 +1,20 @@
# MDC-Ember Changelog

### 0.1.0 Kerry Blue Terrier (November 22, 2021)
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming structure is {{gem / semiprecious stone}} {{dog breed}}, where both tokens begin with the same letter, so this name would need a K-based gem/stone before it. Might I suggest Kunzite?

Copy link
Author

Choose a reason for hiding this comment

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

So Kunzite Kerry Blue Terrier?

Copy link
Contributor

Choose a reason for hiding this comment

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

That name fits the formatting.

@@ -36,6 +37,7 @@ For more information on using ember-cli, visit [https://ember-cli.com/](https://

Thank you to all of our wonderful contributors! Want to be on this list? [Check out our contributing guide!](https://github.com/secondstreet/ember-material-components-web/blob/master/CONTRIBUTING.md) ([emoji key](https://github.com/kentcdodds/all-contributors#emoji-key)):


<!-- ALL-CONTRIBUTORS-LIST:START - Do not remove or modify this section -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to add yourself as a contributor via the CLI.

@@ -38,7 +40,7 @@ export default Component.extend(MDCComponent, SupportsBubblesFalse, {
* @type {Function}
* @param {Boolean} checked
*/
onchange: x => x,
onchange: (x) => x,
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like prettier settings may have changed (please revert them) or it may not have run for this file/PR (please run it).

Copy link
Author

@Gaurav0 Gaurav0 Nov 24, 2021

Choose a reason for hiding this comment

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

I allowed them to change from the old defaults to the new defaults. Please confirm you want non-default settings.

Copy link
Contributor

Choose a reason for hiding this comment

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

If they are supported as options, yes. I'd favor that over changing big chunks of the codebase as part of this dependency update PR.

Copy link
Author

Choose a reason for hiding this comment

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

There is an option for this, but my personal preference is to always use prettier defaults rather than bikeshed. Big chunks of this codebase are being changed anyway.

getNativeControl: () => this.element.querySelector('input'),
forceLayout: () => undefined,
isAttachedToDOM: () => !!get(this, 'element'),
});
},

_sync() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This name is easy to confuse with sync(), which the class gets from this mixin. Perhaps _syncCheckboxState?

//endregion

//region Computed Properties
isOne: equal('size', 1),
isOneDotFive: equal('size', 1.5),
isTwo: equal('size', 2),
isThree: equal('size', 3),
height: computed('size', function() {
height: computed('size', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

See other comment re: prettier

Copy link
Author

Choose a reason for hiding this comment

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

Same comment.

Copy link
Author

Choose a reason for hiding this comment

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

There is no option for this one.

@Gaurav0
Copy link
Author

Gaurav0 commented Nov 29, 2021

Superceded by #146

@Gaurav0 Gaurav0 closed this Nov 29, 2021
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.

2 participants