coreui NoteBox component by dmfalke · Pull Request #1276 · VEuPathDB/web-monorepo

dmfalke · 2024-11-15T19:51:57Z

This PR introduces a NoteBox component. It is meant to be an alternative to Banner, integrating better on content-heavy pages, like record pages.

asizemore

Mostly just suggestions and questions. Otherwise looks really good! And it's 1000x better than what we have.

One big one - I'm not seeing the "Read more"/"Read less" button here, but I believe i should be?

asizemore · 2024-11-20T21:53:55Z

+}
+
+const baseCss = css`
+  border-left: 0.25em solid var(--note-box-border-color);


ive never seen this before. That's very slick

asizemore · 2024-11-20T22:07:59Z

+
+const Template: Story<Props> = function Template(props) {
+  return (
+    <UIThemeProvider


Is there a reason to add the theme here? Or is it just to check that we get the correct colors regardless of theme?

I was just following the same pattern as another story file (can't remember which one). I guess we could take it out, but not sure it hurts?

asizemore · 2024-11-20T22:11:39Z

+  children: ReactNode;
+}
+
+const baseCss = css`


Perhaps additionally a default font? I believe coreui generally uses Inter or Roboto

For content, I think we should use the same font as the rest of the site, but I will take a look at using one of these other fonts.

I think I'm going to leave the font as-is.

asizemore · 2024-11-20T22:12:55Z

+      </div>
+    ),
+  },
+});


A story using a different type of child (for example, an image or some other non-text element) would be helpful if time allows

or an expandable one

Yes, I agree! I will add a couple more, soon.

asizemore · 2024-11-20T22:19:12Z

+      : props.type === 'error'
+      ? errorCss
+      : infoCss;
+  return <div css={finalCss}>{props.children}</div>;


Perhaps for later, but it'd be nice to pair the icon with the NoteBox type. We picked one icon for error, warning, and info ( a looong time ago and i think it was never actually impllemented). Of course, one could pass that as part of the child, but if we want to keep coreui very opinionated then there could be a useIcon prop and if that is true. the appropriate icon for that NoteBox style appears.

asizemore · 2024-11-20T22:43:59Z

-          {warningText}
-        </div>
-      )}
+      {warningText && <NoteBox type="error">{warningText}</NoteBox>}


we should either change warningText to errorText or use the type='warning'. Having "warning" text in an error Notebox is confusing.

This is one of those things where someone wanted the box to be red, even though it isn't strictly an error. I will change it to errorText for clarity.

asizemore · 2024-11-20T22:50:01Z

ahh i found it.

It's tough to understand styling when so much is populated from whatever we're given from the backend! A blessing and a curse 😆

dmfalke · 2024-11-20T23:56:13Z

Mostly just suggestions and questions. Otherwise looks really good! And it's 1000x better than what we have.

One big one - I'm not seeing the "Read more"/"Read less" button here, but I believe i should be?

I fixed a bug related to this. Maybe on this branch? Anyway, I'll take a look.

dmfalke · 2024-11-21T00:46:23Z

Mostly just suggestions and questions. Otherwise looks really good! And it's 1000x better than what we have.

One big one - I'm not seeing the "Read more"/"Read less" button here, but I believe i should be?

I fixed a bug related to this. Maybe on this branch? Anyway, I'll take a look.

Oh, I know what's going on here. This specific case is using a <details> element. It is likely closed by default, which means the overflow detection isn't triggered. This is actually kind of what we want, otherwise there would be two methods to collapse the content.

Generally, I prefer the <details> method, but I added the other one for an ortho use case. I probably should have argued my case 🙂

asizemore · 2024-11-22T13:15:31Z

Ahh okay. Thanks @dmfalke !

Then I think all the rest of my comments are suggestions 👍

dmfalke · 2024-11-22T19:13:29Z

While testing this PR, I noticed a bug that was introduced by #1270:

The persistence of record page table state was also moved to wdk-client. There is at least one place that uses the old updateTableState action, which needs to be updated to use the new action. Additionally, the logic that handles scrolling to the selected table row also needs to be moved to RecordTableSection.

I will make a new issue for this.

A part of this involved increasing padding and border-width to compensate for the additional space needed for the icon.

dmfalke · 2024-11-22T20:22:06Z

I've added icon support, and made some additional stories to highlight the option. I also modified the padding and border width to compensate for the additional space needed for the icon.

asizemore

I was hoping for a good way to align the icon with the text. First i tried the flex + align-items-center route, but that only worked until the text was multiple lines. Next I tried the less elegant way of just adding a negative top margin to the icon. That worked, but requires all of the icons to be the same size. I believe that to be true, but am not 100% certain. What do you think?

asizemore · 2024-11-25T13:47:23Z

+  position: absolute;
+  left: 0.5em;
+  font-size: 1.5em;
+`;


What do you think of adding color: var(--note-box-border-color); ?
I tried it out and thought it looked pretty good!

asizemore · 2024-11-25T13:48:35Z

+  border-left: 0.35em solid var(--note-box-border-color);
  border-radius: 0.25em;
-  padding: 0.5em 1em;
+  padding: 1em 3em;


suggestion: have padding change based on showIcon.

dmfalke added 2 commits November 15, 2024 14:57

Add NoteBox component

8e04c82

Use NoteBox component, and fix overflow detection

8c7ffcc

dmfalke force-pushed the coreui-notebox branch from 163d857 to 8c7ffcc Compare November 15, 2024 19:57

Add story for NoteBox

f4e7ff3

dmfalke requested a review from asizemore November 15, 2024 20:30

Merge branch 'main' into coreui-notebox

de3722b

asizemore reviewed Nov 20, 2024

View reviewed changes

Merge branch 'main' into coreui-notebox

99065db

Update variable name for clarity

7b84bdc

dmfalke mentioned this pull request Nov 22, 2024

Record page - Use new table state actions and state #1285

Closed

Add support for icons to NoteBox.

8f1e79c

A part of this involved increasing padding and border-width to compensate for the additional space needed for the icon.

dmfalke requested a review from asizemore November 22, 2024 20:22

dmfalke force-pushed the coreui-notebox branch from e5e9782 to 8f1e79c Compare November 22, 2024 20:41

Merge branch 'main' into coreui-notebox

888030e

asizemore reviewed Nov 26, 2024

View reviewed changes

dmfalke added 2 commits February 11, 2025 13:19

Merge remote-tracking branch 'origin/main' into coreui-notebox

514aba7

Merge branch 'main' into coreui-notebox

d30c88d

asizemore self-assigned this Feb 26, 2025

Conversation

dmfalke commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asizemore left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asizemore commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmfalke commented Nov 20, 2024

Uh oh!

dmfalke commented Nov 21, 2024

Uh oh!

asizemore commented Nov 22, 2024

Uh oh!

dmfalke commented Nov 22, 2024

Uh oh!

dmfalke commented Nov 22, 2024

Uh oh!

asizemore left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dmfalke commented Nov 15, 2024 •

edited

Loading

asizemore commented Nov 20, 2024 •

edited

Loading