coreui NoteBox component #1276
coreui NoteBox component #1276
Conversation
dmfalke
force-pushed
the
coreui-notebox
branch
from
163d857
to
8c7ffcc
Compare
| } | ||
|
|
||
| const baseCss = css` | ||
| border-left: 0.25em solid var(--note-box-border-color); |
There was a problem hiding this comment.
ive never seen this before. That's very slick
|
|
||
| const Template: Story<Props> = function Template(props) { | ||
| return ( | ||
| <UIThemeProvider |
There was a problem hiding this comment.
Is there a reason to add the theme here? Or is it just to check that we get the correct colors regardless of theme?
There was a problem hiding this comment.
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?
| children: ReactNode; | ||
| } | ||
|
|
||
| const baseCss = css` |
There was a problem hiding this comment.
Perhaps additionally a default font? I believe coreui generally uses Inter or Roboto
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think I'm going to leave the font as-is.
| </div> | ||
| ), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
A story using a different type of child (for example, an image or some other non-text element) would be helpful if time allows
There was a problem hiding this comment.
Yes, I agree! I will add a couple more, soon.
| : props.type === 'error' | ||
| ? errorCss | ||
| : infoCss; | ||
| return <div css={finalCss}>{props.children}</div>; |
There was a problem hiding this comment.
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.
| {warningText} | ||
| </div> | ||
| )} | ||
| {warningText && <NoteBox type="error">{warningText}</NoteBox>} |
There was a problem hiding this comment.
we should either change warningText to errorText or use the type='warning'. Having "warning" text in an error Notebox is confusing.
There was a problem hiding this comment.
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.
|
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 😆 |
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 Generally, I prefer the |
|
Ahh okay. Thanks @dmfalke ! Then I think all the rest of my comments are suggestions 👍 |
|
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 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.
|
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. |
dmfalke
force-pushed
the
coreui-notebox
branch
from
e5e9782
to
8f1e79c
Compare
This PR introduces a
NoteBoxcomponent. It is meant to be an alternative toBanner, integrating better on content-heavy pages, like record pages.