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

Move the logout button in the profile tab? #4876

Closed
albertlast opened this issue Jul 28, 2018 · 30 comments · Fixed by #5701
Closed

Move the logout button in the profile tab? #4876

albertlast opened this issue Jul 28, 2018 · 30 comments · Fixed by #5701
Labels
Milestone

Comments

@albertlast
Copy link
Collaborator

While testing I needed to logout and
realize that the logout button is not in this place where my commonsens it expect.

Reason why i asked if i'm lonely with this or maybe other things the same:
grafik

@illori
Copy link
Contributor

illori commented Jul 28, 2018

i dont think we should in anyway move it. if you hide it there users may think they CANNOT log out at all. this is not the expectation we should set.

@albertlast
Copy link
Collaborator Author

Since it's gui stuff i got no heart about this topic,
but to show why i came up with this,
look here on github, look on google ,facebook where you find the logout button.

@illori
Copy link
Contributor

illori commented Jul 28, 2018

just because they do it, does not mean we need to follow. but i will let others comment.

@wintstar
Copy link

I think the idea is good. If no member is logged in, the login and registration is at the top of the header. If not logged in the registration button appears 2 times. The lower button is the logout button when the user is logged in. Then this button can be placed in the profile menu, as alberlast suggests.

login

@Gwenwyfar
Copy link
Contributor

Gwenwyfar commented Jul 28, 2018

I think it makes sense (and I did the same thing a while back), but I would probably only want to see that once we fix the problem of that menu being on-click, rather than on-hover.

I hardly use any social media so that's not because I got used to them, it simply makes sense that anything related to your account is on your account menu. Then again, it would be inconsistent with what wintstar pointed out, so maybe you could have both.

@Arantor
Copy link
Contributor

Arantor commented Jul 29, 2018

There is a reason I didn't move log out to the profile page - if you do, JS becomes a mandatory requirement. Right now, you can log out without JS (and the profile menu just becomes a link). Move the thing to the menu and you make it absolutely mandatory.

Also has considerations for accessibility.

@Gwenwyfar
Copy link
Contributor

Another reason why that menu shouldn't be onclick. Which is rather annoying really, specially since all others aren't.

@Arantor
Copy link
Contributor

Arantor commented Jul 29, 2018

Feel free to rewrite it then. At the time, onclick was perfectly acceptable given the AJAX load it does.

@Gwenwyfar
Copy link
Contributor

Gwenwyfar commented Jul 29, 2018

The ajax load is actually the problem n1. It is way too slow for a menu, when used in a real server. A plain onclick would at least be okay.

@albertlast
Copy link
Collaborator Author

The least amount of work would be,
to trigger ajax call directly after finish the loading of smf,
like done by crone job.

@Arantor
Copy link
Contributor

Arantor commented Jul 30, 2018

So instead of a slight delay occasionally, you’re going to hit that page every single time?

Then, whatever you do about the profile menu, are you going to do the same to the alerts menu?

@Gwenwyfar
Copy link
Contributor

Gwenwyfar commented Jul 30, 2018

Minor to negligible performance enhancement vs good usability. Take your pick... That's why good engineers don't necessarily make good designers.

And no, those two are fine because the nature of their content is different. If the performance hit on those two were negligible I would remove the ajax there as well, however. And even there, the ajax should only be loading the content itself, not the whole interface.

@Arantor
Copy link
Contributor

Arantor commented Jul 30, 2018

It doesn't load the whole interface. It loads just the HTML that's going to go in the dropdown. Even if you replaced that out and just returned a JSON payload and built the template on the client side, you still have the cost of doing an AJAX call to the system, which because it bootstraps the entire platform isn't super cheap.

However I didn't see a better way to do it that didn't end up duplicating logic and/or hooks - right now it loads the profile area to be able to work out which items are relevant (which includes hooks) and then you can tweak it with a hook. Moving this logic necessarily entails either restructuring the entire profile area or duplicating all of the initial logic in Profile.php to list what areas exist and what permissions are required.

This wasn't done on a whim. I just didn't have anything better - if you have something better than what I came up with, all for it.

@Gwenwyfar
Copy link
Contributor

Which is the whole interface of the "popup", or whatever you want to call it. So even when it is empty, it still loads through ajax, when only the content inside should.

I'm not blaming anyone. This could be better, and that's all I care about.

@Arantor
Copy link
Contributor

Arantor commented Jul 31, 2018

So you want to convert the few hundred bytes of HTML into a few-less hundred bytes of JSON only to recreate the same thing when it has arrived at the other end? The net benefit is absolutely nil in practice because the hit is not the amount of data being sent, but the processing of that data in the first place.

The alternative is to not do it via AJAX and do it inline. Which either means cloning a large chunk of Profile.php somewhere else and running it every page, or transcluding Profile.php on every page. And if paid subs happen to be enabled, forcing a whole extra query every single page.

And if you're doing the inclusion of Profile.php into the menu to avoid duplication of code, that's going to result in multiple additional queries every page that can't readily be avoided except by massively convoluting the already messy profile code.

Why drag Profile.php into it at all? Because the list of things is in no way static and is dependent on what is available to the user, what is configured, what permissions etc. which is why it's descended off the profile code in the first place where this will already have been worked out.

I get that you don't like it. I get that you want to improve it. I just don't think you understand what's actually involved, but at this point you know best, so I'll leave it to you to improve it.

@Gwenwyfar
Copy link
Contributor

Gwenwyfar commented Jul 31, 2018

You assume too much. I never mentioned JSON. Nor did I ever specify anything else you're saying.

If you know a way to do it, then just say so, and stop taking everything personally and putting words that were never said in people's mouth.

@Arantor
Copy link
Contributor

Arantor commented Jul 31, 2018

What's to assume?

You said, and I quote, "the ajax should only be loading the content" - if you're not sending a few hundred bytes of HTML through, you must be sending something else through to build the content which means either JSON or XML, so that some JavaScript can reassemble the HTML when it arrives, assuming some kind of AJAX is actually used.

You didn't specify any of it, and that's kind of the problem. You're saying 'this is terrible, we should change it', and I'm trying to explain that there isn't really a way to fix it without major changes to SMF, or by having downsides that are sufficiently large they aren't recommended, and since it seemed to me that there wasn't an awareness of what the downsides really were, I thought I'd explain, but that just seems like another thing I've screwed up.

In case it wasn't clear, if I had known a better way to do it 5 years ago when I did do this the first time, I would have done so, but I somewhat object to being told that something I did was crap, especially when it's one of the few things I ever did in this project that I felt was an actual improvement rather than running into some kind of brick wall.

But hey, most of the other things I used to be proud of have been pretty demolished in the intervening time, why not this too, eh? (This is why I'm so bitter. Wait until someone comes along and tells you that something you thought you could be proud of is crap and that you simply should have done better, despite not having come up with a better way in 5 years of doing it. Then you have the right to tell me that I should stop taking it personally.)

@Gwenwyfar
Copy link
Contributor

And you're assuming I meant sole data rather than the "content area", which could go both ways.

I said what the problem is, why it it bad, and how it should behave instead. So why exactly are you lashing out instead of trying to help figure out the "how to do it"?

And maybe that's exactly your problem. In the first place, the one who took it personally was you. I really don't care in the slightest who wrote it that way, nor do I know about what you're referring to about your personal affairs.

All you're doing is letting your ego and your pride get in your way. While I understand why you'd feel hurt about it, that is no excuse.

@Arantor
Copy link
Contributor

Arantor commented Jul 31, 2018

  • The list of things that shows in the menu is not in any way fixed. It is based on the current user's permissions, as well as the current site configuration. For example, a site with enabled paid subscriptions and actively configured paid subscriptions will see users get a paid subscriptions button. Similarly if you have 1 or more membergroups that are requestable or joinable (as opposed to only assigned), this option becomes available.

  • In order to display this menu, a large amount of logic - currently in Profile.php - is required. The list of options, their icons, not to mention whether they are enabled and what permissions make them available is laid out in Profile.php.

  • In order so that we don't duplicate all this logic (meaning multiple levels of maintenance), the popup for the user profile calls into the user profile where this will have been decided and gets to reuse it.

  • The choices as presented, then, for populating this menu:

  1. The current situation - have a placeholder link that goes to the profile, and populates its content on-click (in line with the two other menus in the same area)... putting aside 'logout' being up there, the number of times users actually hit this is not generally significant

  2. Populating the content, in whatever fashion, with an AJAX call made after every page load - effectively ramping up the workload since the second HTTP request will be making at least 4, but realistically nearer to 8, queries plus all the processing that will be generated just from loading the page, and doing so on top of all the normal page loading. While the user experience might be improved for the relatively few times the button is clicked, the overall experience will likely be diminished by the increased server load.

  3. Bringing the logic inline and dumping it in something like setupMenuContext. This is potentially viable, so that no additional AJAX is required - but it requires either copy/pasting a large chunk of Profile.php or otherwise duplicating its logic for consistency - into the startup process. If Profile.php is not inlined but called directly at that point, the process will run multiple additional queries that aren't necessary (but note that for proper processing of the menu on most sites at least one additional query will be made, every page load in that situation)

At the time, the hosting landscape was somewhat less generous than now (and even now it isn't particularly generous), so keeping all aspects of load to a minimum is generally a good idea, especially as users frequently do run SMF on free hosts and any significant increase in resource usage will be a huge problem for them.

Honestly, if we're talking about poor UX for loading UI through AJAX, I'd have thought the inline login was a much more significant problem (another thing you can chalk up to me making more terrible), if only because more people will actually encounter it more frequently, and the fact that both cause additional bugs that I don't think I've seen reported in SMF, like what happens if you're logged out, you try to use the login form but things are in maintenance, or you're logged in, then login times out but you then press the profile button and get the login form in the dropdown.

I thought it was on some level a good idea to present some context into this situation, from the one person who actually knows why things were done this way. This started because I thought I could actually do some good - and all I seem to get are lectures about how I'm not good enough or how I should change my attitude - never mind anything else, it's just a sign that I'm doing it wrong.

Go nuts, change it, make it awesome.

@DiegoAndresCortes
Copy link
Member

I even find it hard to locate the login button myself, can't imagine finding the logout button lol
It's a good idea tho but there should be an option for themers to keep this buttons in the menu if they don't want to play around with them.

@Gwenwyfar
Copy link
Contributor

Now that's actually useful, thank you :)

So how it was done in 2.0? Since that menu was part of the main menu.

If you mean the popup login, I have no problem with it. Personally, I don't like it, but no reason to change it. Though I wonder if removing the AJAX wouldn't fix some of those issues.

You should stop assuming people are always "blaming you" for "not being good enough" or whatever is it your ego/self-esteem tell you. Not everything is personal or even directed at you, or at any particular person for that matter.

@Arantor
Copy link
Contributor

Arantor commented Aug 3, 2018

In 2.0 there were no popups except for the help popup (and esoteric things like the spell checker, and all of these were physical new-window popups) - the login and logout links were on the main menu and the login link there was a full page deal. (It's why the small version of the login form is so popular, it's way more convenient than going to a full page only to login.)

And that's the kind of thing that confuses me, though - you have issues with the profile popup being AJAX, but not any of the other popups being AJAX, even though the reality is people will use the others more often and login really has little real justification for being an AJAX popup other than my laziness - there's no dynamic logic or values in there that couldn't be obtained in other ways unlike the profile popup.

Removing the AJAX will fix the side issues I mentioned, which is the product of a state change not communicated to the client and the popup code not being smart enough to handle failed AJAX properly - StoryBB/StoryBB#585 and StoryBB/StoryBB#551 (while the example given is the character dropdown, the same applies to messages and alerts) are both the product of this problem.

@Gwenwyfar
Copy link
Contributor

Gwenwyfar commented Aug 3, 2018

Yes, but it had the profile menu, that's what I'm asking.

You're only going to login once, and the popup is doing the job that would otherwise be a new page load. Same thing with the notifications. You also know what you're getting with them, you're only going to click it if you want to use it. Imagine if all the other menus were like that, it would be a nightmare to use any of them, when all you want is see it as quickly as possible and click a link, or just look over them to see what's there. You're using a menu to move to another page, not to sit and use the content.

That said, ideally the login shouldn't be ajax to open either, nor the notifications. Just the content inside them should be using ajax. For login this would mean the updates after you've entered something. I just don't think they're as problematic.

If it would fix some bugs then that's a good reason to change it as well then.

@Arantor
Copy link
Contributor

Arantor commented Aug 3, 2018

The profile menu was super limited - and mostly hard-coded exactly as you see at sm.org. It's also really easy to manage a menu when you only care about a fraction of the content under it (and none of which requires any real computation to identify)

I dunno about 'only logging in once', I think that's highly dependent on the user and their situation. It's probably more common to 'log in forever' than it used to be because everyone is getting close to having their own devices than before, but it's still not 100% of the time because people do do things like use incognito windows and sneaky time on work computers.

I have to admit I continue to be absolutely confused by your wording. You keep saying "the content inside them should be using AJAX"... it IS. You hit the button, AJAX request is fired, (just the) content comes back and is injected into the right place in the DOM. What else needs to be done? I half wonder how much of this is simply a communication misunderstanding and us talking at cross-purposes because of it.

I really don't understand what you mean by "nor the notifications" - are you suggesting we load all current notifications for the user on every page load for them? Because of the way this got rewritten not so long ago, that's going to be incredibly painful to do.

@Gwenwyfar
Copy link
Contributor

Gwenwyfar commented Aug 3, 2018

I see.

And even so, it would otherwise be a new page load anyway. I don't find it ideal, just not as problematic.

I explained that above: The entire popup block is loading through ajax. That should not happen. It will load through ajax even if the popup is completely empty (meaning: no content, just static buttons and links). I guess that's one case where it's easier to show than explain :P

@Arantor
Copy link
Contributor

Arantor commented Aug 3, 2018

For login and to a lesser degree profile, agreed, they could be solved to not be AJAX, however this isn't trivial in the case of profile. The list of links is somewhat static but by no means completely static because an admin can change something at any time to change the list of items in that list. That's the part I find problematic in solving, especially as you need to be doing database queries for one of them as currently implemented.

For messages and alerts, I don't believe these should ever stop being AJAX driven, just to avoid moving the computational cost to every page rather than just if they are clicked.

This does raise consistency questions, though. People might expect them all to behave consistently.

@Sesquipedalian
Copy link
Member

If we want to make this change, we'd need to do the following in order to make sure the logout button would still be available even when JavaScript is disabled in the user's browser:

  1. We'd need to populate the profile menu on every page load. Most of the current items in that menu can be done using general permissions checks, which are not expensive and wouldn't be problematic. However, as @Arantor says, deciding whether to show a paid subscriptions link would require another query (at least, with the system as it stand now) for something that will rarely ever be used, and that's not worth the cost.
  2. Therefore, we'd need to develop an inexpensive way to decide whether to show a paid subscriptions link—or simply not show that link in the profile menu, but that'd be less than ideal.
  3. We'd need to change the top_section menus to appear on hover instead of on click so that the links in the menu would be available without JavaScript.
  4. For the sake of UI consistency, we'd need to trigger the AJAX calls to populate the message and alert menus on mouseover instead of on click.

We could accomplish point 2 by using a $modSettings variable that tracks the number of active paid subscriptions and updating that variable whenever a subscription's active status changes. Doing the rest ought to be straightforward (albeit a bit tedious) to implement.

@albertlast
Copy link
Collaborator Author

For me i don't understand why we don't mix up the profile pop up,
with dynamic content which came from the ajax side and static content like the logout button.

Todo such "pop up" with css/without js is fare i know possible.

@MechDR
Copy link

MechDR commented Apr 25, 2019

So, is there a conclusion to this feature request, or... ?

@Sesquipedalian
Copy link
Member

It might happen. I still think the approach I described above would work, but I haven't played with trying to implement it yet. If it can be done relatively painlessly, great. If not, then not.

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

Successfully merging a pull request may close this issue.

8 participants