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

Start styling privacy modes ui #126

Merged
merged 6 commits into from
Apr 9, 2021
Merged

Start styling privacy modes ui #126

merged 6 commits into from
Apr 9, 2021

Conversation

cblgh
Copy link
Contributor

@cblgh cblgh commented Apr 8, 2021

mock:
68780Iz

todos:

  • translations
  • hooking up logic
  • stealing @staltz's dropdowny component
  • look into doing the grid stuff with tailwind

@cblgh cblgh mentioned this pull request Apr 8, 2021
14 tasks
@cblgh cblgh force-pushed the privacy-modes-ui branch from 9d03c8c to 092ce82 Compare April 9, 2021 11:26
@cblgh
Copy link
Contributor Author

cblgh commented Apr 9, 2021

aiight this is what it looks like atm:

cc @cryptix @staltz
2021-04-09-135051_1280x890_scrot
2021-04-09-135128_1262x827_scrot

Copy link
Member

@staltz staltz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cblgh Good job! I have a few suggested commits for visual tweaks, and a bigger suggestion. Other than that, this looks visually good and the code refactor of admin/dashboard.go is great.

The bigger suggestion is this: the dashboard page should be reserved for high-level overview of the current state of the room, and most of the time there will be many (or non-zero) peers online, and each of those peers is rendered in a list below the counter. There are plans for making that list dynamic #118, and in that case, this privacy mode UI would jump up and down as peers join and leave, making it difficult to click on the buttons.

So I'd recommend we move all of this new UI to a new page, Settings, with its own left-side menu item, and for now there would only be the privacy mode setting. Note that this page would have to be guarded for permission access. Members cannot change the privacy mode.

web/templates/admin/dashboard.tmpl Outdated Show resolved Hide resolved
web/templates/admin/dashboard.tmpl Outdated Show resolved Hide resolved
web/templates/admin/dashboard.tmpl Outdated Show resolved Hide resolved
@cblgh
Copy link
Contributor Author

cblgh commented Apr 9, 2021

@staltz thanks for the review! :)

regarding moving the logic to a separate page due to planned work, i would suggest that can be done when that work is actually in the pipeline? from #118, it seems that might be coming quite a bit down the road (and end up being left on the cutting floor, due to overriding priorities):

@cryptix: Definitely a version 2.1 thing, though.

if this would have been signaled earlier, i would have taken that into account from the start :~

@staltz
Copy link
Member

staltz commented Apr 9, 2021

regarding moving the logic to a separate page due to planned work, i would suggest that can be done when that work is actually in the pipeline?

There are 3 reasons why to put it on a separate page: (1) #118, (2) Dashboard is for read-only high-level information, not for configuring the room, (3) room-wide configurations should be available only to admins and mods. You're right that (1) is future work, but (2) and (3) are currently applicable. If you need help, we can merge this PR and I can do this page-moving work if you want.

if this would have been signaled earlier, i would have taken that into account from the start :~

We could have communicated better, like from my part the spec uses "Dashboard" to mean "all the pages behind a login", while "Dashboard" in this repo is just one page with some counters.

@cblgh
Copy link
Contributor Author

cblgh commented Apr 9, 2021

Alright, well let's merge this then & perform the larger change in a separate PR 👍. Whether you do that or I do it doesn't really matter as much.

(Edit: Tracking that as a todo in #134)

@cblgh cblgh merged commit f69451d into master Apr 9, 2021
@cblgh
Copy link
Contributor Author

cblgh commented Apr 9, 2021

@cblgh cblgh deleted the privacy-modes-ui branch April 12, 2021 08:10
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.

2 participants