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
feat: Add new Admin Panel #6881
Conversation
This pull request is being automatically deployed with Vercel (learn more). opencollective-frontend – ./🔍 Inspect: https://vercel.com/opencollective/opencollective-frontend/DKF6JmAA752rjbgxv3TDJhDb5knD opencollective-styleguide – ./🔍 Inspect: https://vercel.com/opencollective/opencollective-styleguide/EseG7RR6dEV7qix12u4s8SdWw5Yw |
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.
Not a full reviewed as you're still working on this.
The architecture looks very good; it isolates the legacy stuff nicely.
{ | ||
display: 'flex', | ||
}, | ||
compose(space, layout, flexbox), |
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 personally liked that we matched the original behavior of the rebass/grid
library, and that we're keeping it simple. Why not use the Container
, which supports all these?
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 don't think we were matching the rebass behavior. I'm pretty much sure you can pass alignItems="center" overflow="hidden"
props in rebass.
I'll refactor this to use Container
.
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.
It matches the rebass behavior from when we removed the dependency. This file was initially just a copy-paste of https://github.com/rebassjs/grid/blob/master/src/index.js
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.
Isn't it weird that a div Flex component accepts no CSS flex or div properties?
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.
@kewitz It does accept CSS flex properties already, since it inherits Box
which implements styled-system's flexbox
. Actually, the properties that you added there are already supported by the component.
lang/en.json
Outdated
"AdminPanel.CollectiveSettings": "Collective Settings", | ||
"AdminPanel.FiscalHostSettings": "Fiscal Host Settings", | ||
"AdminPanel.HostDashboard": "Host Dashboard", | ||
"AdminPanel.Menu.BudgetManagement": "Overview", | ||
"AdminPanel.Menu.Collectives": "Collectives", | ||
"AdminPanel.Menu.Expenses": "Expenses", | ||
"AdminPanel.Menu.FinancialContributions": "Financial Contributions", | ||
"AdminPanel.Menu.HostedCollectives": "Hosted Collectives", | ||
"AdminPanel.Menu.Overview": "Overview", | ||
"AdminPanel.Menu.PendingApplications": "Pending Applications", | ||
"AdminPanel.Menu.Reports": "Reports", | ||
"AdminPanel.OrganizationSettings": "Organization Settings", | ||
"AdminPanel.Settings": "Settings", | ||
"AdminPanel.Settings.Updated": "Settings updated.", |
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.
Some duplicated strings in there (eg. Financial Contributions
)
dc6c161
to
9dcaec6
Compare
9dcaec6
to
10f672c
Compare
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.
The mobile version feels fantastic ✨ Some feedback below, but I think we're mostly good for internal testing.
/betree/admin
turns out empty when accessed directly
When accessed via URL, it works but the menu entry is not selected:
Co-authored-by: Benjamin Piouffle <benjamin@opencollective.com>
Co-authored-by: Benjamin Piouffle <benjamin@opencollective.com>
Co-authored-by: Benjamin Piouffle <benjamin@opencollective.com>
Co-authored-by: Benjamin Piouffle <benjamin@opencollective.com>
Resolves opencollective/opencollective#4693
:slug/admin
pageThis is still not the default settings menu, you'll need to add
/admin
at the end of your collective URL to access it.