-
Notifications
You must be signed in to change notification settings - Fork 5
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
Implement a simple user settings page #190
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start! See suggestions below.
|
||
{% block main %} | ||
<section id="user-settings"> | ||
<h2>{{ page_title }}</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be a <h1>
and moved out of the <section>
. It would also be nice to add sr-only class to the h1 tag.
<h2>{{ page_title }}</h2> | |
<h1>{{ page_title }}</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we not supposed to signal somehow to non-screenreading users that they are on the user settings page, @podliashanyk ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added 95df103 to resolve most of your comments, but it also adds a card title that reads "User settings" - but I don't think this is a good solution either, especially if we may resort to adding multiple cards for settings groups.
Again, we should resolve how to signal to an able-eyed user which page they are currently on, without forcing them to rely on reading the URL in the address bar - but it doesn't seem the current incarnation of argus-htmx has any kind of page heading system (maybe because we've only been focused on implementing the incidents list thus far).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Page heading system is a good idea.
We could use breadcrumbs. Or we could configure the typography plugin in our project so that making <h1>
visible will apply proper styling to it. I prefer the latter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added typography plugin in a separate PR #202
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan of breadcrumbs myself. And "prose" will not work with cards and forms inside, will it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prose
wouldn't be an issue for any standard card
or form, however for daisyUI's menu
, table
, link
etc it will break the styles.
Edit: prose
sets max width to 65ch
so I would consider this as annoying default for card
and forms as well as other components
We could also consider dividing settings into categories and adding for example a drawer to the layout. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this as is and work on polish later.
95df103
to
46a65de
Compare
Agreed, though the introduction of djLint on main didn't make that easier :P |
I rebased this on the latest main, tried to fix my changes to be compliant with the new formatting/linting rules introduced, but it seems the linter workflow is still mad about lots of HTML that I did not introduce in this PR... |
This copies the current user settings menu from the user dropdown to the new user settings page, but leaves out the logout link and styling. Further styling work is needed.
As per review suggestions.
The main branch introduced djLint formatting rules after this feature branch was created. This reformats the compounded HTML introduced in this branch to comply with the new rules.
46a65de
to
dc55329
Compare
Master has reportedly been fixed, rebased and force-pushed this. |
Butt-ugly first draft of a user settings page, more or less based on the current user drop-down menu.
I'm going to need some styling help here, because Tailwind is just confusing and downright counter-intuitive to an old-timer like me ;)
Closes #170