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

Added setup option to disable timeago #125

Merged
merged 5 commits into from
Jan 12, 2016
Merged

Added setup option to disable timeago #125

merged 5 commits into from
Jan 12, 2016

Conversation

balsdorf
Copy link
Contributor

@balsdorf balsdorf commented Jan 8, 2016

Our internal users do not like time ago so I added a setup option to disable it. It is enabled by default.

@glensc
Copy link
Member

glensc commented Jan 8, 2016

it should came from user prefs instead, like noted in original PR implementing the feature: #116

having it configured in two places makes it confusing, altho i like the default coming from setup, i.e could be useful for new users, so maybe leave your change in, but add per user preference as well?

and timeago should be always there, just default should be human readable vs raw, so having it off you could still obtain timeago by clicking or hovering.

so what you think? as if so, could do user pref later, still keep this PR as is

@glensc
Copy link
Member

glensc commented Jan 8, 2016

actually, i went ahead and just implemented the disabled state behaviour, so just user preference option missing

Changed setup references to "timeago" to "relative_date"
@balsdorf
Copy link
Contributor Author

Thanks for implementing the disabled state. I went ahead and made it a user preference.

I also decided to remove "timeago" from the actual setup file name, changed it to "relative_date" so we aren't tied to one specific library name in the future

@glensc
Copy link
Member

glensc commented Jan 12, 2016

there's seem to be mix of enabled, 1 and 0 values. should stay consistent, use everywhere enabled or the numbers, i would prefer numbers, as we may have in the future value 2 for some specific behavior.

and as i look the code, imho it's broken as to dom you write enabled, but in main.js you check for 1

@balsdorf
Copy link
Contributor Author

setup uses "enabled" while user preferences uses 1 and 0. I don't like it either but wanted to stay consistent with each section. Changing setup to use 1 and 0 was beyond the scope of this PR.

I write the value of upr_relative_date to the DOM which will be 1 or 0. For existing users I check if "enabled" == 1 but for new users I was just using the string so that was broken. Fixed.

@glensc
Copy link
Member

glensc commented Jan 12, 2016

you still need to cast it to int. while boolean true converted to string is 1, then boolean false converted to string is empty string.

glensc added a commit that referenced this pull request Jan 12, 2016
Added setup option to disable timeago
@glensc glensc merged commit be5d297 into master Jan 12, 2016
@glensc glensc deleted the timeago_toggle branch January 12, 2016 20:43
@glensc glensc added this to the 3.0.8 milestone Jan 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants