-
Notifications
You must be signed in to change notification settings - Fork 73
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
fix: page titles, routes, and sidenav consistency, add file storage logs page #4757
base: main
Are you sure you want to change the base?
Conversation
🤖 This is an automated code coverage reportTotal coverage (lines): 33.95% |
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.
For starters, this is definitely a huge improvement in both clarity and function. My only note is about semantics/future-proofing it a bit more.
Right now the new siteNav
object is playing triple-duty: it's being used to route components, it's being used to map paths to titles, and it's being used to determine the sidebar display for a specific set of routes. I think it's probably okay to keep that all together but maybe worth renaming it something like siteRoutes
if it's serving all those functions.
The use in frontend/routes.js
suggests a mapping between the component name and path name but doesn't enforce it. It's worth looking into whether something like this would work to establish a single-source of truth.
SiteRoutes.map(route => (
<Route path={route.path} element={route.Component} />
))
5cf7858
to
1d799e1
Compare
2bf0f72
to
93d6590
Compare
two separate commits to make reviewing a little easier: https://github.com/cloud-gov/pages-core/pull/4757/commits |
addressed suggestion and added new functionality
5f68feb
to
4235662
Compare
if we approve and merge andrew's work on file details view first, i'll update that view to have a mini file history table to match. |
@sknep let's not add additional feature work to this PR. |
4235662
to
cc7aae5
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.
Looks good to me. 🎖️
cc7aae5
to
9f0274d
Compare
sorry @apburnes I forgot to change the published builds text. It's fixed now but it invalidated your approval. however... now it looks like this :( resizing the sidebar for this seems like overkill. I wanted to keep "Published" because it's in the path/route, but Preview Branches fits better: And now I wanna change the icon because it really don't make sense with "preview". Thoughts? |
@sknep what icon are you thinking? Also, technically the "Published branches" is not just preview but also production and demo branches. Maybe "Site branches" could work to keep the text on one line and be more semantically accurate. |
Changes proposed in this pull request:
/logs
security considerations