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.

Sign up for GitHub

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

check config for defaults #52

Merged
merged 1 commit into from
Mar 14, 2018
Merged

check config for defaults #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions addon/mixins/touch-action.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
import { get, defineProperty, computed } from '@ember/object';
import Mixin from '@ember/object/mixin';
import { isHTMLSafe, htmlSafe } from '@ember/string';
import config from 'ember-get-config';

const defaults = config.EmberHammertime || {};

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;
Copy link
Member

Choose a reason for hiding this comment

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

Don't use typeof, and use === e.g.:

const TouchActionOnAction = (defaults.touchActionOnAction === undefined) ? true : defaults.touchActionOnAction;

// Remove 'onclick' if you do not want the styles automatically applied to elements with an `onclick`
const TouchActionAttributes = ['onclick'];
const TouchActionAttributes = defaults.touchActionAttributes || ['onclick'];
// Remove whichever element types you do not want automatically getting styles applied to them
const TouchActionSelectors = ['button', 'input', 'a', 'textarea'];
const TouchActionSelectors = defaults.touchActionSelectors || ['button', 'input', 'a', 'textarea'];
// 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;';
Copy link
Member

@eriktrom eriktrom Mar 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

The app config is already used in that stuff, just not in the mixin, I think.

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

@eriktrom we're already consuming the app config in index.js here https://github.com/html-next/ember-hammertime/blob/master/index.js#L55.

Copy link
Member

Choose a reason for hiding this comment

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

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!)


function touchActionStyle() {
let style = get(this, 'touchActionProperties');
Expand Down Expand Up @@ -38,7 +41,7 @@ export default Mixin.create({
ignoreTouchAction: false,

init() {
this._super();
this._super(...arguments);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@eriktrom why do we not want this?

Copy link
Member

Choose a reason for hiding this comment

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

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 👍)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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


const {
tagName,
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"broccoli-merge-trees": "^2.0.0",
"broccoli-stew": "^1.5.0",
"ember-cli-babel": "^6.6.0",
"ember-get-config": "^0.2.4",
"hammer-timejs": "^1.1.0"
},
"devDependencies": {
Expand Down
Loading