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

Improve admin UI around roles #1702

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Improve admin UI around roles #1702

wants to merge 16 commits into from

Conversation

nike4613
Copy link
Contributor

@nike4613 nike4613 commented Jan 1, 2025

All role-related logic is moved into the "Roles" admin tab, and the search bar is now sticky-scrolling above searchable submenus. Role layering description improved, and algorithm overview of role distribution added.

I added the algorithm overview as HTML in the language file; is there a better way to have large translatable strings?

@TimGoll
Copy link
Member

TimGoll commented Jan 2, 2025

I added the algorithm overview as HTML in the language file; is there a better way to have large translatable strings?

Not really yet. I wanted to add a base-text system for that, that supports headings, text and bullet points. But so far, I did not have the time to do any of that. But I'm not a fan of using HTML in the UI, because the rendered text looks completely out of place.

Speaking of it, could you show screenshots of what you did? In general your changes sound like a solid idea! I think we already talked about this on discord.

For the future: Stuff like this could easily be separated into multiple smaller PRs. But I struggle with that as well, so I get why you did it like this.

@nike4613
Copy link
Contributor Author

nike4613 commented Jan 2, 2025

Stuff like this could easily be separated into multiple smaller PRs

Yeah, the multiple changelog entries tipped me off there, but by that point it was already done. I can pull the overview page and description changes out easily enough if you want.

But I'm not a fan of using HTML in the UI, because the rendered text looks completely out of place.

Nor am I, but the overview doc needs nested lists. I'm sure I could do it in vgui, but I think I'd rather spend the time doing HTML or Markdown to vgui automatically.

Speaking of it, could you show screenshots of what you did?

Here's the video I recorded the other day of the UI changes without the overview page: https://files.catbox.moe/zn3kqz.mkv (MKV w/ AV1; if browser/media player doesn't work, use mpv)

I can take more screenshots later, not super sure when. Only things new from that video are the overview page (looks like the changelog pages) and text changes in the role layering page.

@nike4613
Copy link
Contributor Author

nike4613 commented Jan 3, 2025

Screenshots:

image
image
image
image

@TimGoll
Copy link
Member

TimGoll commented Jan 3, 2025

Ok, I watched the video and took a look at the screenshots. First of, it's awesome what you did here.

However, to me it feels a bit "janky"(?). I absolutely understand why you did it the way you did it, but it feels weird to have entries above the search bar. That looks wrong to me on first glance. It looks a bit out of place.

I'm not sure if this needs some color to differentiate it from the rest. Or if it should be below the search bar and the "pinning" should be shown in a different way.

Side note, this is how winXP did the pinning:
image

And win10 had the search bar at the bottom:
image

Not sure if these windows solutions point into a better direction, but I think we should open a discussion about the design here. Because your implementation seems a bit unintuitive to me. Albeit be it very impressive from a technical point of view.

@TimGoll
Copy link
Member

TimGoll commented Jan 3, 2025

mockup

Here's a quick mockup of the ideas I had in mind. There's absolutely room for improvement here, I just wanted to show something so we have a base for discussions

@TimGoll
Copy link
Member

TimGoll commented Jan 3, 2025

On a technical side I'm wondering how you did it that some submenus are "pinned", and some are normal menus. Personally I'd have added a new flag to it, something like:

CLGAMEMODESUBMENU.pinned = true

or

function CLGAMEMODESUBMENU:IsPinned()
    return true
end

@Histalek
Copy link
Member

Histalek commented Jan 3, 2025

First of all: amazing work! Especially the terrifically in-depth description for the rolelayering, I appreciate it!

Stuff like this could easily be separated into multiple smaller PRs

Yeah, the multiple changelog entries tipped me off there, but by that point it was already done. I can pull the overview page and description changes out easily enough if you want.

It might be worth to split off the overview page for now. I'm quite worried about how feasible it is to translate this properly
But i also don't want your in-depth description to go to waste or for my concern to block your other changes

I'd also appreciate it if some translators could chime in if my concern is warranted in the first place

@nike4613
Copy link
Contributor Author

nike4613 commented Jan 3, 2025

@TimGoll: ...

I have 2 arguments for why the current presentation is the correct choice:

  1. By consistency. All other menus (having all items being searchable) have, and have always had, the search bar at the top, above the items that are searched. It follows then that a search bar searches items listed below it.

  2. By literary convention. Consider a heading:

    Some Text

    Heading

    More Text

    The underline under the heading connects the heading to the text after it, on the other side of the line. The search bar UI already has a highlight line underneath, so this connection is maintained.

Windows 10 is an interesting comparison, but the search bar is on the taskbar, not at the bottom. The taskbar can be placed on any edge of the screen. Plus, the menu is invoked by first selecting the search bar, making it a somewhat different situation to our UI. I will also reference several other applications for placing the search bar above the searched content:

  • Visual Studio
  • VSCode
  • Rider
  • PowerToys Run
  • Explorer
  • KDE Plasma (basically everything)
  • Thunderbird
  • Every search engine ever
  • ...

I strongly suspect that, one way or another, placing the search bar above the content it searches is the way to go. There might be something worth doing to highlight more that the items above it aren't searched, though I also think that there's not much point, as even in the worst case, the user tries to search and immediately finds that the items above are not filtered.

On a technical side I'm wondering how you did it that some submenus are "pinned", and some are normal menus.

It's implemented as a new function on CLGAMEMODEMENU returning a list of submenus which are 'non-searched'. The logic here is 2-fold:

  1. This method is compatible with the previous approach of GetVisibleSubmenus() + HasSearchbar(), and the defaults are such that other menus continue to behave exactly as before with no changes.
  2. This method is compatible with the existing approach to searching, just with some slightly different setup in the submenu list. (I personally don't like this search approach, and would personally do it differently, but no sense in changing what works IMO)

It would be very possible to also have an IsPinned option on the submenu itself, and adjust the default logic to continue to preserve behavior, if that's desirable. For the purposes of the roles menu, there's not much point, because we treat the auto-detected submenus fundamentally differently from the role list anyway, so we already have the searchable and non-searchable menus split out from each other.

@TimGoll
Copy link
Member

TimGoll commented Jan 3, 2025

I understand your point about the search bar position. And I agree with your reasoning. Although I'd like to add that my suggestion to move it to the bottom would apply to all menus in TTT2, not just this single one.

I guess it all boils down to the fact that I don't like anything above the search bar. I guess that is the part that annoys me the most. We probably need someone else to chime in on this as well, I don't want to be the only one to comment on this. Maybe I'm the only one with this opinion and your solution is actually better.

But with your feedback in mind, I'd chose my design with the searchbar on top, like it has always been. I honestly don't care if the search includes the pinned entries or not, as long as it is clear that these are pinned entries - if this makes sense.

We should also keep it in the back of our mind that the same system could probably be applied for the shop settings as well.

@NickCloudAT
Copy link
Contributor

If nobody minds my opinion on this:

I like how nike made this. I think putting the searchbar at the bottom is a bad design solution in this case, but that's also just my taste.

But: The searchable content should always be the last thing in the overall list. Also, above a searchfield, I would like to see a "split" line, pretty much like this:

image

@nike4613
Copy link
Contributor Author

nike4613 commented Jan 7, 2025

What about something like this?

image

I think I prefer the line on the bottom, but I don't hate this.

@TimGoll
Copy link
Member

TimGoll commented Jan 7, 2025

To me it has the same problem still. Having a search bar that is neither at the top or bottom looks wrong to me.

My suggestion with a searchbar on top would look like this:
mockup2

Note that the searchbar stays there even when scrolling. I personally don't care if the search function only searches through the roles or through the pinned submenus as well

@nike4613
Copy link
Contributor Author

nike4613 commented Jan 8, 2025

To me it has the same problem still. Having a search bar that is neither at the top or bottom looks wrong to me.

My suggestion with a searchbar on top would look like this:

Note that the searchbar stays there even when scrolling. I personally don't care if the search function only searches through the roles or through the pinned submenus as well

I showed this to a friend who has never used this UI in its current state, and his immediate first assumption given this screenshot was that the pinned items were user-controlled. I think this explains much of my own reservation about the design: Since the "pinned" items are not user controlled (and IMO "pinned" is the wrong term to use for them anyway), it does not make sense to keep them int he visual scope of the search bar. I believe that @NickCloudAT's problem is that the visual scope of the search bar is not clear enough. (I have also shown it to one other person for whom that was the problem.)

I'm not super sure how much bikeshedding is worth putting into this bit of the UI anyway, considering it's an admin menu. Even if it isn't immediately clear from just seeing the UI, interacting with it should clear up potential confusions, esp. for the people who will actually be using it.

@saibotk
Copy link
Member

saibotk commented Jan 8, 2025

I think both top and below pinned is okayish (with a separator).
Personally id also prefer it at the top because thats just less disruptive and id expect a search there.
Also dont we atm always show these searches at the top by default in other menues? Then id definitely say lets keep it this way for consistency with other Menues to provide a predictable UI.

And yes its not worth discussing too much about it.

Id say @TimGoll you can just go ahead and select the solution you see for TTT2 as you have he greater vision for our UI and this just needs a decision from someone.

Thanks @nike4613 for your efforts and your patience with this discussion! 🫶

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

Successfully merging this pull request may close these issues.

5 participants