check config for defaults #52
Conversation
|
This LGTM! 🚀 |
sbatson5
force-pushed
the
check-config-for-defaults
branch
from
c39bc66 to
ba81f8b
Compare
|
@sbatson5 the config stuff should work. What issues are you experiencing? |
|
Ah, I guess the issue is the config values work when you don't use the mixin, but the mixin does not? |
I think he wants to configure it from environment.js from within the app. If that's not it then not sure either... |
|
|
||
| const FocusableInputTypes = ['button', 'submit', 'text', 'file']; | ||
| // Set this to `false` to not apply the styles automatically to elements with an `action` | ||
| const TouchActionOnAction = true; | ||
| const TouchActionOnAction = (typeof defaults.touchActionOnAction == 'undefined') ? true : defaults.touchActionOnAction; |
There was a problem hiding this comment.
Don't use typeof, and use === e.g.:
const TouchActionOnAction = (defaults.touchActionOnAction === undefined) ? true : defaults.touchActionOnAction;
| @@ -38,7 +41,7 @@ export default Mixin.create({ | |||
| ignoreTouchAction: false, | |||
|
|
|||
| init() { | |||
| this._super(); | |||
| this._super(...arguments); | |||
There was a problem hiding this comment.
remove ...arguments - this is a very hot code path - hat tip to @runspired for noticing this in a previous round where i did the same thing. U don't need it here, nor do u want it.
There was a problem hiding this comment.
@eriktrom why do we not want this?
There was a problem hiding this comment.
runspired told me to remove it when i updated this file last year - he said that ...arguments slows things down(which is true, i later found out when writing an unrelated algo), and that its not needed either in this case (b/c its a mixin? that part im not sure 'why' but i trust him on it 👍)
There was a problem hiding this comment.
I haven't heard of spread arguments being a perf hit before. Or is it just because of the babel transpilation? Happy to revert this back however as it shouldn't make a difference.
There was a problem hiding this comment.
babel transpilation per this discussion (of this file) 2 years ago though, hazy memory - #27 (comment), didn't quote it quite right, actually quoted this instead (a couple comments down) - #27 (comment)
(side note per my comment of my more recent encounter with passing ...arguments to a helper fn, inside an O(n) loop, perf_hooks caught it, i have not dug deeper, it was pretty recent finding, passing named args manually fixed it)
hope that clarifies things - for both of us now haha
| // The actual style string that is applied to the elements. You can tweak this if you want something different. | ||
| const TouchActionProperties = 'touch-action: manipulation; -ms-touch-action: manipulation; cursor: pointer;'; | ||
| const TouchActionProperties = defaults.touchActionProperties || 'touch-action: manipulation; -ms-touch-action: manipulation; cursor: pointer;'; |
There was a problem hiding this comment.
There is a corresponding AST transform that we would want to line up with this. Either remove configuration of this, or make that use the same configuration passed in from the apps environment.js. Otherwise styles will be out of sync depending on whether the element is dynamic(rendered by glimmer vm) or static (rendered by browser as normal html tag). We want them to be the same in both cases.
There was a problem hiding this comment.
The app config is already used in that stuff, just not in the mixin, I think.
There was a problem hiding this comment.
yeah - i don't know if ember-get-config will pass through its options to the index.js file where we forward those options to the AST parser... or how to reconcile that.... but we shouldn't allow native link or button elements to have different css classes than ember components (like link-to) as that'll cause an inconsistent experience for devs (unless they're completely aware of touch-action css property, which is funky to say the least)
I'll check this as reviewed by me, but this issue in particular, I think we should ensure is right before merging...
There was a problem hiding this comment.
@eriktrom we're already consuming the app config in index.js here https://github.com/html-next/ember-hammertime/blob/master/index.js#L55.
There was a problem hiding this comment.
ahh - yes thank you. looks like this will work out then using ember-get-config for the mixin, and that code for the ast parser.. config set once in environment.js. nice
(double rainbow, it does exist!)
|
I think this should work. Going to merge. |
|
Thanks @sbatson5! |
|
ps @sbatson5 - double thanks never hurts - nice job fixing this issue - its been outstanding for a good long while, appreciate it |
What's in this PR?
Should address the comment on this thread: #31 (comment)
I ran into this issue as well. The README implies that by adding the
EmberHammertimehash to your config/environment you can set defaults. However, the mixin does not look at the consuming app's config and overwrite the defaults. This PR would allow the user to specify which defaults they want.