JohnMcLear / ep_page_view

Etherpad Page View plugin
https://npmjs.org/package/ep_page_view
14 stars 17 forks source link

Checking 'Page View' setting when URL param 'pageview' is 'true' #24

Closed lpagliari closed 9 years ago

lpagliari commented 9 years ago

It seems that 'Page View' checkbox is not being checked when using the URL param 'pageview'.

To reproduce:

Expected: 'Page View' setting should be checked Actual: setting is not checked

This PR should fix that. Tested on Chrome and FF.

JohnMcLear commented 9 years ago

I think it's being overridden by a user defined cookie? Which would kinda be what you want no?

lpagliari commented 9 years ago

If that is the case, we need to fix the code too (in another way, of course): currently if I have the cookie set to not show PV and use the URL param as true, it's enabling PV but not checking the checkbox, which is also inconsistent.

Regarding the precedence between URL param and cookie, I thought URL param should have the higher one, but now I'm not sure. Thinking as a user, I would be mad to have to uncheck PV setting every time I opened a pad and wanted PV to be disabled.

Thoughts?

JohnMcLear commented 9 years ago

Agreed

lpagliari commented 9 years ago

Sorry, so which direction should I take? Make URL param have higher precedence, or the cookie?

JohnMcLear commented 9 years ago

I'd say the cookie so we give more control to the user but we should allow it to be "forced" by the site admin if required. Afaik we already allow for it to be forced in settings.json

lpagliari commented 9 years ago

So on this scenario:

When user opens a pad, should PV be enabled? I would expect so, but wanted to confirm.

JohnMcLear commented 9 years ago

If the plugin sets it on by default but the user has turned off page view and the URL sets pageview to off then it shouldn't show as page view.

lpagliari commented 9 years ago

OK, got it, @JohnMcLear . cff5480 should address that.