Skip to content
This repository has been archived by the owner on May 24, 2022. It is now read-only.

Port/code reuse use case #143

Merged
merged 11 commits into from
May 9, 2022
Merged

Port/code reuse use case #143

merged 11 commits into from
May 9, 2022

Conversation

st0nebreaker
Copy link
Contributor

@st0nebreaker st0nebreaker commented May 4, 2022

Becca Steinbrecher added 3 commits May 4, 2022 15:20

Verified

This commit was signed with the committer’s verified signature.
abelsromero Abel Salgado Romero

Verified

This commit was signed with the committer’s verified signature.
abelsromero Abel Salgado Romero

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@st0nebreaker st0nebreaker added the team/content-platform Content Platform Team related tickets. label May 4, 2022
@st0nebreaker st0nebreaker added this to the AR - Sprint 3 milestone May 4, 2022
@st0nebreaker st0nebreaker self-assigned this May 4, 2022
@st0nebreaker st0nebreaker requested review from bretthayes and zlonko May 5, 2022 17:48
@st0nebreaker st0nebreaker marked this pull request as ready for review May 5, 2022 17:48
@zlonko zlonko mentioned this pull request May 5, 2022
Copy link
Contributor

@zlonko zlonko left a comment

Choose a reason for hiding this comment

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

Thank you for bringing in so many improvements here, from Tailwind preparations to accessibility considerations. I have just a few observations below.

Reviewing this PR helped me notice a few things to address in the Code Health PR. I think it would be helpful to have this merge first, so that I can pick up ThreeUpText and the alt-text addition to BlogListItem.

Comment on lines 121 to 125
<blockquote className="blockquote mt-8 px-6">
<p className="border-left-red pl-3 font-weight-bold">
&ldquo;For our new developers, Sourcegraph has been invaluable to get to know the
repository structure, to track down where code lives, and self-service during their
investigations.&rdquo;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am just noticing that these are a similar pattern to BlockquoteWithBorder. What do you think of using that component in these sections?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Except if we conform these to our BlockquoteWithBorder they'll lose the bold and the side border length will be longer (past the author section). Do you think these are small enough differences we should still do it? Ss below, top is how it would change, bottom is what it is now. Cc: @bretthayes what do you think?
image

Copy link
Contributor

@zlonko zlonko May 6, 2022

Choose a reason for hiding this comment

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

Good point! It is helpful to see the contrast. I am okay with these differences if using components increases maintainability overall, but I defer to you and @bretthayes' judgment. We could also handle this in a separate issue if this is outside of scope here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I'd like to re-use components where possible. I applied that BlockquoteBorder component to this page and refactored the rendering a bit. So it should be good now if there aren't any reservations!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the initial intent was to have a border against the quote itself, but with the border extending down towards the author, it creates more of a separation between the left side content and the blockquote. I would say this is a nice tradeoff to enable more code re-use. Funny that this is PR is for a page about code-reuse. 😆

We can also revisit this a bit more later cause I'm anticipating we'll have a blockquote refresh along with some other UI components. ✨

Copy link
Contributor Author

@st0nebreaker st0nebreaker May 6, 2022

Choose a reason for hiding this comment

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

Yea I'd prefer to have the border just by the quote, not the author. And we can have all quotes with the border conform to this. A blockquote refresh is needed I think!

Copy link
Contributor

@bretthayes bretthayes left a comment

Choose a reason for hiding this comment

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

Looks good Becca! Just a few revisions below. Might need to do a final pass with prettier as well. 🤓

Comment on lines 121 to 125
<blockquote className="blockquote mt-8 px-6">
<p className="border-left-red pl-3 font-weight-bold">
&ldquo;For our new developers, Sourcegraph has been invaluable to get to know the
repository structure, to track down where code lives, and self-service during their
investigations.&rdquo;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the initial intent was to have a border against the quote itself, but with the border extending down towards the author, it creates more of a separation between the left side content and the blockquote. I would say this is a nice tradeoff to enable more code re-use. Funny that this is PR is for a page about code-reuse. 😆

We can also revisit this a bit more later cause I'm anticipating we'll have a blockquote refresh along with some other UI components. ✨

Copy link
Contributor

@zlonko zlonko left a comment

Choose a reason for hiding this comment

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

Looks great, @st0nebraker! 💫

@st0nebreaker st0nebreaker merged commit 504e230 into main May 9, 2022
@st0nebreaker st0nebreaker deleted the port/code-reuse-use-case branch May 9, 2022 14:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
team/content-platform Content Platform Team related tickets.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Port over Use Case: Code Re-Use
3 participants