-
Notifications
You must be signed in to change notification settings - Fork 65
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
Unified style and accessibility upgrades for login, registration, and profile pages #4491
base: master
Are you sure you want to change the base?
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.
Joe, I think something's been missed, this is how I manually tested it:
- log in (works so far, and was taken to new back office log in page)
- select 'Profile' from drop down menu on top right
- was taken to a front of house template, which was themed (testing with ?theme=clean allowed me to change the theme).
either I've misunderstood the scope of this issue, or something's gone wrong. I was expecting the profile page to be back office.
OK, thanks for testing it. Not sure why you're not seeing the right templates, but I'll go ahead and assign to Andy in case there is something different about the way you're testing 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.
Thanks for this, its good to reduce the number of overall templates.
I've added some comments inline. Some more general comments:
- If you enter your username and password incorrectly when the page reloads the form is larger, this feels very strange and should be a consistent experience for the user.
- Can you review all of the comments throughout this PR and ensure they're appropriate. See for example
settings.css
I don't think the comment here is useful. - The forms feel a bit lost in the middle of the screen, especially on wider screens.
- These forms introduce a layout system of their own, is there a reason we're not utilising the established layout for the back office here?
Thanks for these comments and the chat we had, which helped clarify a few things for me. I've opted to left-align everything to be more consistent with other back-office pages, and I've returned to using the title-section element. This puts the semantic name of each of these pages in an h1, which is good (was h2 in the first version), but it means we need to resolve the h1 in the header before this will be accessible, as discussed.
I've tried to use what I can, but there are some float-based components that were not feasible to re-use. I've opted to marginally expand the utilities so that we can develop a framework-agnostic set of layout tools with modern patterns like grid and flex, while we shift frameworks. Let's discuss this more because I have a few things coming down the pike, and I'd like to expand the utilities in the back office more formally. |
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.
- ✅ log in takes me to the new back office page.
- ✅ failed / rejected log in remains in back office.
- ✅ clicking 'forgotten your password' on new back office log in page takes me to back office
- ❌ clicking 'register' on new back office log in page takes me to front of house
- ❌ register from navigation bar takes me to front of house
- ❌ once logged in, the profile link in account menu from nav bar takes me to front of house
nb. log in looks quite different to me than in the screenshots above:
as per screen share discussion, this could be a domain v path issue.
I still can't replicate this issue @StephDriver. Are we sure your browser isn't caching the old pages? @mauromsl or @ajrbyers, could you tell me what about Steph's dev setup might cause this issue? I can't think of a scenario in which this view Line 382 in 042b33e
would return a template under |
@StephDriver let’s find some time to dig around this next week. |
I don't think so... mostly because I tried it with multiple different browsers and had same results, and there were two pages which loaded the new versions. But I agree it needs further digging... so watch this space. |
Closes #4380.
Implementation notes
Limitations
core/base.html
-- it results in two h1s in most cases, and one of them is often empty. My preference is that h1 is used for the actual page, not the site name, and that the site name is simply appended to the title element, so it is available in the browser tab.