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

Settings description on installation #197

Closed
Destroy666x opened this issue Jan 6, 2014 · 22 comments
Closed

Settings description on installation #197

Destroy666x opened this issue Jan 6, 2014 · 22 comments
Assignees
Milestone

Comments

@Destroy666x
Copy link
Contributor

@Destroy666x Destroy666x commented Jan 6, 2014

I just tested the MyBB 1.8 installation and one thing is still really missing for me - descriptions for settings, especially these for cookies.

Why? It isn't hard to notice cookies are the biggest issue for new users in MyBB. Days without support questions connected with them are a rarety. This topic has around 1 500 000 views: http://community.mybb.com/thread-126359.html

So I think you should add cookie settings examples or anything similar (+maybe other settings too). I see no reason why would that be bad.

@nmalcolm
Copy link
Contributor

@nmalcolm nmalcolm commented Jan 6, 2014

So I think you should add cookie settings examples or anything similar

The values set in the installer are based on what MyBB assumes is correct (taken from the hostname and path). Also if you click on the question mark next to "Cookie settings" it'll take you to the docs page which has other examples.

The biggest issue with cookies is that you have to supply the domain and path yourself. MyBB tries to take the smart route and detect them automatically but when users try to switch domains or move from localhost to a web host they run in to issues. Perhaps a Peeker for the cookie settings and an "If you are unsure, leave these as they are" message would work so they don't run in to issues initially. There likely are some who see "domain" and try to correct it by removing the leading period, etc.

@Destroy666x
Copy link
Contributor Author

@Destroy666x Destroy666x commented Jan 6, 2014

There likely are some who see "domain" and try to correct it by removing the leading period, etc.

That proves my point. Users don't know what they are configuring, so a description would help them. Going to external sites isn't a good solution comparing to a short note that can be simply added below setting or at least in tooltip.

As for moving, I agree, I also thought about possible solutions for this but couldn't think of any.

@nmalcolm
Copy link
Contributor

@nmalcolm nmalcolm commented Jan 6, 2014

If the user doesn't know what it is they shouldn't touch it. There isn't really a simple way to explain how cookies work and if they're not able to figure it out from the values then a short note probably wouldn't make much difference. That's why we link to the docs page which explains everything they need to know. The sticky in General Support is for post-installation issues like the scenarios I mentioned. Currently we educate users quite well on how to configure cookies. As long as they're manually configured there will be potential issues due to lack of knowledge.

@nmalcolm nmalcolm modified the milestone: 1.8.0 Mar 21, 2014
@DiogoParrinha DiogoParrinha added this to the 1.8 Beta 2 milestone May 31, 2014
@DiogoParrinha
Copy link
Contributor

@DiogoParrinha DiogoParrinha commented Jun 26, 2014

@Destroy666x anything you'd like to do here or should I reject this?

@Destroy666x
Copy link
Contributor Author

@Destroy666x Destroy666x commented Jun 26, 2014

Well, I can add at least description for cookie settings if this is accepted.

But first someone please check if this this pull request is ok so I can add other later: #787 Haven't worked much on github yet, so not sure whether branches etc. are correct, thus sent only one PR for now.

@DiogoParrinha
Copy link
Contributor

@DiogoParrinha DiogoParrinha commented Jun 26, 2014

Sure I'll try that out today.
You should create branches off 'feature' (if you're working on feature fixes or feature additions) like the workflow suggests, i.e. you create branch featurefix-197 for this one and then pull request by comparing it against the MyBB repo's 'feature' branch. As you'll always branch off your repo's 'feature' branch, you need to keep it in sync with MyBB's (to do that you'll need to use the Git Bash).

If you need help, I may create an internal thread for helping out.

@DiogoParrinha
Copy link
Contributor

@DiogoParrinha DiogoParrinha commented Jun 27, 2014

@Destroy666x I have merged your other PR.

@Destroy666x
Copy link
Contributor Author

@Destroy666x Destroy666x commented Jun 28, 2014

Ok, thanks for tips and checking. I'll add other PRs soon.

I was thinking about this in general and I'm wondering why can't we simply read the cookie domain/path globally instead of relying on settings? Maybe it's a stupid question and I'm missing something obvious, but I don't really understand why not. Code for it doesn't look heavy so that's not a problem. Three installation inputs less, users not worried about something that can be done automatically even when they move to another hosting (of course assuming the detection from installation works for all domains).

@DiogoParrinha
Copy link
Contributor

@DiogoParrinha DiogoParrinha commented Jun 28, 2014

What do you mean by "read domain/path globally" ?

I'm postponing this to Beta 3 as Beta 2 ETA is closing in.

@DiogoParrinha DiogoParrinha modified the milestones: 1.8 Beta 3, 1.8 Beta 2 Jun 28, 2014
@euantorano
Copy link
Member

@euantorano euantorano commented Jun 28, 2014

We can't read the domain/path automatically globally as it adds additional overhead and removes useful functionality found in previous releases.

@Sama34
Copy link
Contributor

@Sama34 Sama34 commented Jul 10, 2014

Perhaps a Peeker for the cookie settings and an "If you are unsure, leave these as they are" message would work so they don't run in to issues initially.

I think this would be the easies yet best solution. @Destroy666x you still working on this?

@Destroy666x
Copy link
Contributor Author

@Destroy666x Destroy666x commented Jul 10, 2014

@Sama34, yes, I'll start working on my assignments tommorow. I'll probably add something like alerts with description when user tries to focus textboxes of these settings.

But I still don't really understand why can't it be done automatically.

it adds additional overhead

Which wouldn't matter even on the biggest forums since the code is very quick already and it can be shortened (by setting it one time and then only checking if it's correct). Inefficient coding like 4 usergroup WHERE INs in one query adds most likely more overhead.

removes useful functionality

I don't think it's useful on more than 5% forums and for those the settings could be left as an 'override' in ACP.

@Sama34
Copy link
Contributor

@Sama34 Sama34 commented Jul 11, 2014

I don't think it's useful on more than 5% forums and for those the settings could be left as an 'override' in ACP.

So those 5% of administrators should sign-in to their DB and modify the settings there? 1.8 is not really the stage to make this kind of modifications, not at this point at least.

@JordanMussi
Copy link
Member

@JordanMussi JordanMussi commented Jul 13, 2014

I support adding a Peeker that sends a message when the user edits the settings saying "Only experienced users should edit this option. Do you wish to revoke your changes?" (with a link to revoke changes). That should hopefully discourage users from editing it...

@DiogoParrinha
Copy link
Contributor

@DiogoParrinha DiogoParrinha commented Jul 16, 2014

@Destroy666x
Copy link
Contributor Author

@Destroy666x Destroy666x commented Jul 17, 2014

I have all my changes done on localhost, only need to PR them.

@DiogoParrinha
Copy link
Contributor

@DiogoParrinha DiogoParrinha commented Jul 17, 2014

@Destroy666x we need that PR then :P

@Destroy666x
Copy link
Contributor Author

@Destroy666x Destroy666x commented Jul 17, 2014

Will do tonight.

Destroy666x added a commit to Destroy666x/mybb that referenced this issue Jul 19, 2014
Warnings when users try to change settings which often cause problems.
Also a JS password comparison.
Destroy666x added a commit that referenced this issue Jul 19, 2014
@Destroy666x Destroy666x added the fixed label Jul 19, 2014
@Destroy666x
Copy link
Contributor Author

@Destroy666x Destroy666x commented Jul 19, 2014

This is done. Works perfectly for me so merged it immediately as I don't really want to solve any conflicts again..

@DiogoParrinha
Copy link
Contributor

@DiogoParrinha DiogoParrinha commented Jul 19, 2014

Works for me.

@Orel-A
Copy link
Contributor

@Orel-A Orel-A commented Jul 21, 2014

You should consider using onfocus event instead of both onkeyup and onchange events.

@Destroy666x
Copy link
Contributor Author

@Destroy666x Destroy666x commented Jul 21, 2014

I made it with onfocus first, but I tested and I came to conclusion that it's better this way - the warning should only show when the user actually changes the value. Additionally, the revert link requires it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.