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

FI-3019: Add custom blockquote styles #556

Merged
merged 3 commits into from
Nov 29, 2024
Merged

FI-3019: Add custom blockquote styles #556

merged 3 commits into from
Nov 29, 2024

Conversation

AlyssaWang
Copy link
Contributor

@AlyssaWang AlyssaWang commented Nov 13, 2024

Summary

Adds a bar on the left of blockquotes.

Screenshot 2024-11-13 at 2 50 53 PM

Open to feedback about appearance, e.g. bar should be thicker!

Testing Guidance

Visual test with Demo Suite -- blockquotes should show up with a bar on the left.

@AlyssaWang AlyssaWang requested a review from arscan November 13, 2024 19:53
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

Attention: Patch coverage is 61.90476% with 8 lines in your changes missing coverage. Please review.

Project coverage is 83.97%. Comparing base (18bc135) to head (03774f4).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...c/components/SuiteOptionsPage/SuiteOptionsPage.tsx 33.33% 2 Missing ⚠️
...tails/TestGroupListItem/NestedDescriptionPanel.tsx 33.33% 2 Missing ⚠️
...ite/TestSuiteDetails/TestListItem/TestListItem.tsx 33.33% 2 Missing ⚠️
...nfigMessagesDetails/ConfigMessagesDetailsPanel.tsx 50.00% 1 Missing ⚠️
...nents/TestSuite/TestSuiteDetails/TestGroupCard.tsx 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #556   +/-   ##
=======================================
  Coverage   83.97%   83.97%           
=======================================
  Files         262      262           
  Lines       11430    11430           
  Branches     1260     1260           
=======================================
  Hits         9598     9598           
  Misses       1822     1822           
  Partials       10       10           
Flag Coverage Δ
backend 92.17% <ø> (ø)
frontend 79.19% <61.90%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arscan
Copy link
Contributor

arscan commented Nov 26, 2024

It does look a little too subtle to me. Let's look to see what it is in this interface for a comparison:

This is a block quote

This is another line of block quote

Based on this, I would recommend a thicker line and maybe even making quoted text a tad lighter as a visual cue (only if meets accessibility guidelines though)? Not that GitHub is some kind of authority on this, but it does agree with my impression.

let lineIsQuote = false;
const quotes: string[] = [];
// Separate description into sections of blockquotes and non-blockquotes
descriptionLines.forEach((line) => {
Copy link
Contributor

@arscan arscan Nov 26, 2024

Choose a reason for hiding this comment

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

Wow this is a lot more complex than I thought it would have to be. Why can't this whole file just be something like this (a very cursory search resulted in this, apologies if there is an obvious reason why this isn't supported):

     <Markdown 
        components={{
          blockquote: ({node, children}) => (
            <blockquote style={{borderLeft: "4px solid #ccc", paddingLeft: "1rem"}}>
              {children}
            </blockquote>
          )
        }}
      >
        {markdown}
      </Markdown>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason is that blockquotes on different lines appear as separate blockquote components, so the bar on the left would render separately per line — assuming that it would be part of one blockquote, we’d actually want only one bar to render, hence the complicated logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I mean by the way:

Screenshot 2024-11-26 at 1 07 51 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

What was your input to that? I'd expect there to be a blank between the lines if it was like this:

> This is a blockquote

> This is another blockquote

vs. this:

> This is a blockquote
>
> This is another blockquote

That's consistent with what github does:

Checking what this does:

Separate 1

Separate 2

VS putting in the GT sign on a blank line.

Separate 1

Separate 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah I was checking against the first one -- I can simplify things then

@AlyssaWang
Copy link
Contributor Author

Updated styling, new version looks like this:

Screenshot 2024-11-26 at 1 18 33 PM

This color works with a11y contrast standards so it should be good to go.

@AlyssaWang AlyssaWang requested a review from arscan November 26, 2024 18:19
@@ -17,6 +17,14 @@ class DemoGroup < Inferno::TestGroup

This is a dummy canonical link http://hl7.org/fhir/ValueSet/my-valueset|0.8 that should not be
interpreted as a table

> This is a blockquote

Copy link
Contributor

@arscan arscan Nov 26, 2024

Choose a reason for hiding this comment

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

Suggested change
>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to update

@AlyssaWang AlyssaWang requested a review from arscan November 26, 2024 19:29
@arscan
Copy link
Contributor

arscan commented Nov 26, 2024

Not to iterate this to death playing code golf... but... is there a reason we can't just set a css rule for blockquote globally at this point? Just 1 css rule put in the right spot, no other code needed? I'm having trouble thinking of anywhere else that we have blockquotes, and even if we did, why wouldn't we just want this to be default styling everywhere for a blockquote.

@AlyssaWang
Copy link
Contributor Author

Not to iterate this to death playing code golf... but... is there a reason we can't just set a css rule for blockquote globally at this point? Just 1 css rule put in the right spot, no other code needed? I'm having trouble thinking of anywhere else that we have blockquotes, and even if we did, why wouldn't we just want this to be default styling everywhere for a blockquote.

Yeah that's true.. should be updated in the latest push

Copy link
Contributor

@arscan arscan 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! Always a win to have a bit less code to maintain.

I committed an example back in the demo suite because I think its helpful to have that just for reference.

@AlyssaWang AlyssaWang merged commit e2773e9 into main Nov 29, 2024
9 of 10 checks passed
@AlyssaWang AlyssaWang deleted the FI-3019-blockquotes branch November 29, 2024 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants