[QUO-325] Clean up blank underline block #1595

lindsaykwardell · 2024-01-12T23:04:41Z

Change some minor styles based on feedback from Stacey.

🔧 Modifying a component

Context

https://github.com/NoRedInk/NoRedInk/pull/46846#issuecomment-1883598283

🖼️ What does this change look like?

Component completion checklist

tesk9 · 2024-01-13T01:38:34Z

src/Nri/Ui/Block/V6.elm

+                        Underline ->
+                            [ Css.textDecoration Css.underline
+                            , Css.property "text-decoration-thickness" "1px"
+                            , Css.property "text-underline-offset" "4.5px"


Perhaps 0.2rem or something like that instead? so that this style works well for 30px fonts and 18px fonts.

4.5px looks a bit far to me for an 18px font:

The main issue I was running into was getting it aligned with a blank underline, which is why I was moving it at all. Happy to go toward a different solution, I wasn't attached or happy with how this was turning out.

Okay! I think I'm following. So border-bottom is used for adding a "blank" underline and text-decoration is used for adding underlines to actual text?

I think this brings me back to my question from yesterday: "Should non-blanks ever be underlined?" Like, is the intention of the underline just to be a different styling for a blank? If so, then I don't think we'd ever need text-decoration at all? (Although I think an API change could clarify the expected behavior a bit.)

Like, is the intention of the underline just to be a different styling for a blank?

Yes. In our context, students thought the dashed bordered rectangle versions of the blanks meant that it was interactive (they're not)

How about this?

30px 18px

Instead of adjusting the border offset for the text, I adjusted the line height of the blank, bringing it up rather than down. I also set the border/underline to 0.1rem, which at a default zoom is rendering at 1.5px.

Oops, missed the first part of Tessa's question. We don't underline text in Short Response. I'm not 💯 sure but don't think there's a use case for it in scaffolding or Guided Tutorials either? (the other 2 places currently using Block)

I was just aiming for some consistency between the two modes. Taking away the names the component is currently using, we have two display modes that we want to be able to choose between:

Show blanks with dashes

Show blanks with underlines

Originally, I felt it made more sense to make that a global thing: show everything with dashes, or show everything as underlined. If we are feeling this is not the case, I can scope that down to just blanks.

Personally, I think we should not show everything as underlines. The case that really clinches it for me is a blank within an emphasis, which is the one you've got screenshots of up above in this thread:

I would expect the "underline" style to look like this:

Not like this:

Ah, I get where the other underlines came from. We want to apply underline to just blanks. Or apply to everything, but have the border go all the way around except in blanks, where it should just be on the bottom.

We don't currently have a use case to change the dashes on the other Block elements. The border is there for added contrast in addition to the background color.

Fine with me! Here's the update (which looks just like Tessa's example):

tesk9 · 2024-01-13T01:46:51Z

src/Nri/Ui/Block/V6.elm

@@ -542,7 +542,7 @@ toMark config { backgroundColor, borderColor } =
                    Underline ->
                        Css.solid

-            notBottomBorderColor =
+            borderColor_ =


Do we definitely want to use a different color for the underline than the text?

No, we definitely want the text color to apply to the underline. I was not finding a solution that felt good for that, because I wasn't finding where the text color is set inside the component. I was assuming it came from outside.

Ahh I see! This is happening because of the user agent stylesheet.

In the screenshot above, there's a label, so the blank is treated as highlighted by "fruit." Semantically, we're using the mark tag for this:

If there's no label or emphasis, the styles apply as expected:

(I don't have an opinion on whether this is worth addressing as part of this PR, but if you do override the user agent stylesheet, please check that high contrast mode keeps working.)

tesk9 · 2024-01-13T01:54:41Z

src/Nri/Ui/Block/V6.elm

+                config.labelCss
+                    ++ (case ( config.borderStyle, config.content ) of
+                            ( Underline, [ Blank _ ] ) ->
+                                [ Css.top (Css.px 6)


I don't think adjusting the top px will work consistently for a couple of reasons.

The first reason is variable font size. As far as I am aware, the Block component is used in containers that set the font to 30px and in containers that set the font to 18px. I believe the place the underline is going to be used has 18px font.

30px 18px 12px

Original Spacing

New Spacing

Users should be able to customize their font size without all our styles breaking. But even if we don't set that as a goal, I'm not sure that the 6px from the top will land in the right spot, you know?

The second reason I think we should change approaches here is that setting the top to 6px will sometimes play badly with repositioning:

previous behavior top: 6px

Code for the above screenshots

On #1594 or with it merged in, adjust the first repositioning example to use:

, Block.view [ Block.plaintext "gifts" , Block.label "direct object collision" , Block.yellow , Block.labelId directObjectId , Block.labelPosition (Dict.get directObjectId offsets) ] , Block.view [ Block.plaintext " " ] , Block.view [ Block.label "long adjectives shouldn't hit a lil" , Block.underline , Block.purple , Block.labelId adjectiveId , Block.labelPosition (Dict.get adjectiveId offsets) ]

A third reason is that when labels aren't repositioned, the 6px change will make them look a little funky when they're rendered next to normal labels:

CC @staceyadams

@lindsaykwardell let's revert this part.

@tesk9 for context of why we wanted to change this: the labels look more detached in the use in Short Response because we use the blank in 3 sentences in a column, sometimes with label, sometimes without. In the third sentence frame example, you can see that it appears equidistant between the second and third bullet so not immediately visually clear at a glance that it belongs to the third one.

Reverted this change.

tesk9 · 2024-01-16T21:11:39Z

Looks good! @lindsaykwardell would you please review/merge #1594 so that the changes are a little easier to double check?

lindsaykwardell · 2024-01-16T21:15:56Z

Done!

…erline-block

tesk9

Looks great! Thanks!

Someone may want to update the corresponding Paper document still (although I'm not sure who owns that work at this point! Not me, I'm pretty sure 😆)

tesk9 · 2024-01-16T22:42:16Z

I thought I'd double-check high-contrast mode, and just FYI the experience is a little confusing when the blank appears within an emphasis. If we're planning to use the new underline blank styles with emphasis, this is something we might want to improve.

	Standard contrast	High contrast
Blank
Blank in emphasis
Mixed content

lindsaykwardell · 2024-01-17T19:27:53Z

I just pushed up a change to support the correct background in high contrast mode, although I'm not 100% certain how to enable it locally to test.

tesk9 · 2024-01-17T19:31:29Z

I just pushed up a change to support the correct background in high contrast mode

Yay, thank you!!

I'm not 100% certain how to enable it locally to test.

We have an instructional guide for testing in high contrast mode! 🎉

staceyadams · 2024-01-17T19:33:43Z

I can own updating the Paper doc

linear · 2024-01-17T23:26:57Z

QUO-325 Update blank styling on sentence frames

Cleam up blank underline block

4e22242

lindsaykwardell changed the title ~~Cleam up blank underline block~~ Clean up blank underline block Jan 12, 2024

lindsaykwardell requested review from staceyadams, a team, juanedi, ArthurJordao, tesk9 and mfonism and removed request for a team January 12, 2024 23:07

staceyadams approved these changes Jan 12, 2024

View reviewed changes

Clean up the emphasezed underline style

badaaeb

tesk9 reviewed Jan 13, 2024

View reviewed changes

Lindsay Wardell added 2 commits January 16, 2024 09:26

Remove adjustment to label on blank underline

fc91fdb

Stop underline/dashed from affecting the emphasis border

40fda93

tesk9 added 2 commits January 16, 2024 15:25

Merge remote-tracking branch 'origin/master' into quokka/clean-up-und…

a58ff1f

…erline-block

Remove the blank style setting from the example by default

a27b319

tesk9 approved these changes Jan 16, 2024

View reviewed changes

adjust background color for high contrast mode

bbbf930

lindsaykwardell merged commit 7db3f8e into master Jan 17, 2024
8 checks passed

lindsaykwardell deleted the quokka/clean-up-underline-block branch January 17, 2024 21:16

lindsaykwardell mentioned this pull request Jan 17, 2024

Patch block to have correct styles #1599

Merged

lindsaykwardell changed the title ~~Clean up blank underline block~~ [QUO-325] Clean up blank underline block Jan 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[QUO-325] Clean up blank underline block #1595

[QUO-325] Clean up blank underline block #1595

lindsaykwardell commented Jan 12, 2024 •

edited

Loading

tesk9 Jan 13, 2024

lindsaykwardell Jan 16, 2024

tesk9 Jan 16, 2024

staceyadams Jan 16, 2024 •

edited

Loading

lindsaykwardell Jan 16, 2024 •

edited

Loading

staceyadams Jan 16, 2024

lindsaykwardell Jan 16, 2024

tesk9 Jan 16, 2024

staceyadams Jan 16, 2024

lindsaykwardell Jan 16, 2024

tesk9 Jan 13, 2024

lindsaykwardell Jan 16, 2024

tesk9 Jan 16, 2024

tesk9 Jan 13, 2024

lindsaykwardell Jan 16, 2024

staceyadams Jan 16, 2024 •

edited

Loading

lindsaykwardell Jan 16, 2024

tesk9 commented Jan 16, 2024

lindsaykwardell commented Jan 16, 2024

tesk9 left a comment

tesk9 commented Jan 16, 2024

lindsaykwardell commented Jan 17, 2024

tesk9 commented Jan 17, 2024

staceyadams commented Jan 17, 2024

linear bot commented Jan 17, 2024

[QUO-325] Clean up blank underline block #1595

[QUO-325] Clean up blank underline block #1595

Conversation

lindsaykwardell commented Jan 12, 2024 • edited Loading

🔧 Modifying a component

Context

🖼️ What does this change look like?

Component completion checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staceyadams Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

lindsaykwardell Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staceyadams Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tesk9 commented Jan 16, 2024

lindsaykwardell commented Jan 16, 2024

tesk9 left a comment

Choose a reason for hiding this comment

tesk9 commented Jan 16, 2024

lindsaykwardell commented Jan 17, 2024

tesk9 commented Jan 17, 2024

staceyadams commented Jan 17, 2024

linear bot commented Jan 17, 2024

lindsaykwardell commented Jan 12, 2024 •

edited

Loading

staceyadams Jan 16, 2024 •

edited

Loading

lindsaykwardell Jan 16, 2024 •

edited

Loading

staceyadams Jan 16, 2024 •

edited

Loading