-
Notifications
You must be signed in to change notification settings - Fork 114
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
Walkthrough for new label UI #791
Walkthrough for new label UI #791
Conversation
+ Make room for walkthrough button on Label navbar + Add walkthrough tour targets and content
www/js/diary/infinite_scroll_list.js
Outdated
var d = $q.defer(); | ||
$ionicScrollDelegate.scrollTop(true); | ||
d.resolve(); | ||
return d.promise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we please use standard javascript promises instead of $q
? I don't see anything here that native promises don't support and I don't want to add dependencies on libraries that are already obsolete unless I have to.
https://github.com/kriskowal/q
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I just saw $q
on the tour library reference and assumed that was the standard way to do it.
<button ng-repeat="selF in filterInputs" ng-click="select(selF)" class="col col-50 button labelfilter" ng-class="{on:selF.state}" style="text-align: center;font-size: 14px;font-weight: 600;" translate> | ||
{{selF.text}} | ||
</button> | ||
<button ng-click="resetSelection()" class="col col-50 button labelfilter last" ng-class="{on:allTrips}" style="text-align: center;font-size: 14px;font-weight: 600;" translate> | ||
{{'.show-all'}} | ||
</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The advantage of using entries such as col-50
is that they (at least theoretically) auto-resize on different size screens. Can't you use a lower level of col
- e.g. col-33
etc instead of hardcoding values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are lots of options!
https://ionicframework.com/docs/v1/components/#grid-explicit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intent wasn't to hardcode values, it was to have the buttons automatically resize to fit the text within them, which they do now. Is that objectionable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I first got to this code, there was a width
parameter stored in each infinite_scroll_filter
. I changed it to use the col
values because that was hardcoded (and not accommodating of multiple languages), but my new solution seems even less hardcoded.
+ That plus a few final tweaks to the walkthrough itself
www/js/diary/infinite_scroll_list.js
Outdated
var d = $q.defer(); | ||
$ionicScrollDelegate.scrollTop(true); | ||
d.resolve(); | ||
return d.promise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the replacement
var d = $q.defer(); | |
$ionicScrollDelegate.scrollTop(true); | |
d.resolve(); | |
return d.promise | |
return new Promise(function(resolve, reject) { | |
$ionicScrollDelegate.scrollTop(true); | |
resolve(); | |
}); |
+ Go back to using col-XX for width + Use Promise instead of $q
Walkthrough for new label UI
Adds a walkthrough button and tour for the new label UI. Planning to make this play the first time a user views the new UI; haven't done that yet.