-
Notifications
You must be signed in to change notification settings - Fork 123
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
347 use component in account settings and staff pages #354
347 use component in account settings and staff pages #354
Conversation
…s/edit use of new component
Hey @MooseCowBear! Thank you for taking care of this :) Nice find with the CSS and the colors! Something is really mixed up here and I don't know what. I will revert my change for now and look into it again. Can you share what browser are you using? It works fine for me on Chrome. Regarding the |
Those screenshots are Safari, but I've tried it in Chrome and Firefox as well and I get the same results across all three. As far as the page component, I take it that the reason the component is only intended for the dashboard views is because the styling in the non-dashboard views is different? If that's the case, one option is to go ahead and use the page partial for both and write helper methods to produce the correct styling classes depending on whether the current user is staff or not. Then the page partial can be thought of as enforcing consistency among views of each type. The other alternatives that come to mind are adding conditional rendering to the registrations/edit view or making an exception and not using the pages partial for that view, both of which seem less than ideal to me. (Although one could argue that just not using it for the registrations/edit page might be the simplest way to go.) |
@Ptrboro @MooseCowBear yes thank you for taking initiative here! This is strange. #348 fixed the layout for me. I don't know why it would be different in different machines but this has been plaguing from the start. I also note that colours look different on other computers compared to mine. I use Chrome on Ubuntu. Regarding the edit user settings for adopters and fosters, how about if they also use the dashboard layout? They need a dashboard too, and it's something I want to discuss with other leads but it might be easiest if all users use the same dashboard layout. |
If adopters will end up with their own dashboard layout, I can just leave the overlap untouched for now and let it resolve with the layout change. Is there any way that the css discrepancy could be a cache invalidation issue? I'm just thinking about how it magically changed on my computer. |
This PR might have fixed some things related to the layout. #365 |
Thank you! |
🔗 Issue
#347
✍️ Description
Draft of update to app/views/devise/registrations/edit.html.erb, app/views/organizations/invitations/new.html.erb, and
app/views/organizations/staff/index.html.erb to use the new dashboard/page component.
📷 Screenshots/Demos
One side effect of using the new component is overlap in the navbar of signed in adopters due to the position absolute on the navbar-transparent class. As seen here:
One fix might look like:
Which is what is included in the current state of this pull request.
Another option is to give more contrast between the navbar and the title for the page component, like:
I don't know which looks better. Or maybe someone has ideas for a better option?
Also, #348 is breaking the layout for me.
This is without merging:
After merging:
One thing that is interesting about #348 is that the colors of the icons are the Bootstrap primary color which is blue (which was also the color they were for me when I made the change in #331) and not the GeeksUI primary purple-ish color. The dashboard layout using id="page-content" instead of a container class seems to work when the links and icons are the GeeksUI purple. Which makes me wonder if something is happening with the css that's causing the discrepancy...