Skip to content
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(ui): Release Details Sidebar component update #29842

Merged
merged 5 commits into from
Nov 12, 2021

Conversation

mikellykels
Copy link
Contributor

@mikellykels mikellykels commented Nov 6, 2021

Update design of release details sidebar and add new component to keep the sidebar headers/content/etc consistent. Update the existing release details sidebar sections with the new component.

https://www.figma.com/file/kvA3xwx5D7dLuOljIJffvc/Audits?node-id=2%3A68
https://www.figma.com/file/jsZJ9v8ditpm4uBISafH9P/Releases-V4?node-id=1241%3A16163

cc @robinrendle

FIXES WOR-1313

Before

Screen Shot 2021-11-05 at 5 57 22 PM

After

Screen Shot 2021-11-05 at 5 50 05 PM

Screen Shot 2021-11-05 at 5 50 22 PM

Update design of release details sidebar and add new component to keep the sidebar headers/content/etc consistent.

FIXES WOR-1313
@mikellykels mikellykels requested a review from a team as a code owner November 6, 2021 01:01
@mikellykels mikellykels requested review from a team and robinrendle November 6, 2021 01:01
Comment on lines 12 to 15
/**
* Used to add a new sidebar section on Release Details page.
*/
function ReleaseSidebarSection({title, children, icon, ...props}: Props) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design of sidebar sections and titles is used all over the app (transaction summary, project details, alert rule, ..) not just release details. Should we make something reusable so that we can use it in those places too?
I think this refactor should be done with the whole app in mind, not just release details.
cc @vuluongj20

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure, I can update the naming here - I mostly made it to where it should work on any sidebar in the app, but I only updated the Release Details page. Should I start working on updating other pages with this after?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renaming sounds like a good idea 👍

@@ -85,27 +86,28 @@ class CommitAuthorBreakdown extends AsyncComponent<Props, State> {

return (
<Wrapper>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the SidebarSection have some predefined margin so that they are easily stackable on top of each other?
We could get rid of all these Wrapper components and ensure that the spacing is consistent everywhere.

@matejminar
Copy link
Member

Sweet, I believe you can now kill static/app/views/releases/detail/overview/styles.tsx too.

Copy link
Member

@matejminar matejminar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏👏👏 Code looks good to me.

Please do not forget to document this component in our storybook (either in this or follow-up PR).

@mikellykels mikellykels merged commit 57faeac into master Nov 12, 2021
@mikellykels mikellykels deleted the kelly/release-details-sidebar branch November 12, 2021 21:55
@github-actions github-actions bot locked and limited conversation to collaborators Nov 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants