-
Notifications
You must be signed in to change notification settings - Fork 38
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
Update theme #123
Update theme #123
Conversation
src/pages/layouts/main.hbs
Outdated
{{{body}}} | ||
</div> | ||
{{#if admin}} | ||
{{> admin_subnav}} |
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 merged the main and admin templates into one—they were mostly the same save for the admin subnav showing up conditionally. I mad that condition a parameter of the main theme.
Just an observation on the test site - this looks really nice, and as a bonus, seems to also resolve the weirdness that causes the buttons to show as white text on white on my iOS browser for the existing default theme. Nice work! |
We talked about this a bit on IRC today but yeah this is looking crisp @johnholdun, thank you, and it's a big relief to see things resizing properly on mobile as well. If you have a chance to get that admin nav stuff appearing before the admin page content like we discussed I think I'd be very happy to merge this along with the first PR. |
if I say fixes #81 in a comment, will it link things properly? I use Jira at work, sob update: okay I just edited john's PR text. I probably wouldn't do that to most people but I know he'll forgive me and/or get me back in the future. |
Oh, is there an IRC? |
not right now! John and I have just been in a group chat that never left IRC, so it's our primary way of talking to each other. |
8418a65
to
8b13217
Compare
Added another commit here that changes the order of the admin nav to appear before main content on small screens, and makes the admin links programmatic so that the link to the current page is unlinked and gets a different visual treatment (on all screen sizes). The changes are on the Glitch if you want to see. I squashed #123 into the first commit here and forced-push so I think that this can be merged after that one without conflicts, but I'll re-rebase if that ends up not being true. |
@johnholdun it appears to be not true, sorry. One more rebase and I can merge! I'll get committer access set up for you & the other current contributors soon and then we can use branches in the repo that we can all collaborate on more easily. |
8b13217
to
f277022
Compare
Haha I messed that up so bad. Rebased again though! |
thanks! I'm so happy to have a mobile-friendly view now 😊 |
(This builds on #120 and #122 so please review just the last commit. I'll rebase as those are updated and merged. Thanks!)
Adds some enhancements to the theme/UX. There are some DX changes as well, like making sure all the colors in the CSS file use variables. This is mostly subjective, so if there's anything I should alter or roll back, please don't hesitate to ask!
See it live here, and ask me privately for the password if you want to see the admin stuff: https://befitting-innate-roadway.glitch.me
[ed. note: fixes #81 and fixes #4 btw! -ckolderup]