Skip to content

feat: add noticebox component - #19

Merged
1 commit merged into
alphafrom
add-noticebox
Apr 8, 2020
Merged

feat: add noticebox component#19
1 commit merged into
alphafrom
add-noticebox

Conversation

@ghost

@ghost ghost commented Apr 7, 2020

Copy link
Copy Markdown

This adds the NoticeBox component. See the design-system entry, the latest specs and a slack conversation about it.

This is what the component looks like at the moment (screenshots from storybook):

Screenshot 2020-04-07 at 11 41 12

Screenshot 2020-04-07 at 11 41 19

Screenshot 2020-04-07 at 11 41 26

Todo:

  • Add overflow x auto to content container (talked to Joe about this, setting overflow x auto would also not wrap text, which we don't want. So instead of setting overflow x, we're delegating responsibility for oversized content to the dev implementing the noticebox, who can create a wrapper or set styles to prevent it)
  • Revisit NoticeBoxIcon. I went with the current approach just to get things going, and to circumvent some issues I was encountering with ui-icons.
  • Fonts from the specs haven't been set yet. I see roboto regular and roboto medium mentioned, and was wondering whether we use weights to set weight, or use the font-family to indirectly set weight (the roboto npm package uses weights: https://github.com/KyleAMathews/typefaces/blob/master/packages/roboto/index.css, which we use in the app platform. So weights seems correct)
  • Add unit tests
  • Add e2e tests

Remarks:

  • Can't import an outline <Info> icon from ui-icons. There are two Info icons being exported, are we overwriting the outlined version maybe?

@ghost
ghost changed the base branch from alpha to alpha-fix-story-prefixes April 7, 2020 09:31
@ghost ghost changed the title Add noticebox feat: add noticebox component Apr 7, 2020
@ghost
ghost changed the base branch from alpha-fix-story-prefixes to alpha April 7, 2020 10:11
@ghost
ghost force-pushed the add-noticebox branch from 31ab02a to 15f7fb8 Compare April 7, 2020 10:11
@cooper-joe

Copy link
Copy Markdown
Member

This looks great and all according to specs 👍 I observed a minor issue with overflow-x which I discussed with Ismay on slack, now added to todo list.

@ghost
ghost force-pushed the add-noticebox branch from 2878185 to 8d4ef2f Compare April 8, 2020 09:13
@ghost
ghost marked this pull request as ready for review April 8, 2020 10:32
@ghost
ghost self-requested a review as a code owner April 8, 2020 10:32

@HendrikThePendric HendrikThePendric left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you rewrite the stories to the new syntax, i.e. with a default export + named exports?

Also I noticed we are using a title prop for the title. I don't think that is a bad approach per se, but I do see some similarities between this component and the Modal for which we expose a ModalTitle. So you could say this is somewhat inconsistent. Not sure if we should change that....

@ghost

ghost commented Apr 8, 2020

Copy link
Copy Markdown
Author

Could you rewrite the stories to the new syntax, i.e. with a default export + named exports?

Yeah I thought about that, but I'd prefer to do that all at once for the entire lib. I already see several conventions that are diverging in this lib (like jest tests with a .test vs tests in the __test__ folder, the storybook syntax, etc.). I would rather move everything over at once than do it gradually for each PR, I feel like otherwise we'll have multiple conventions in the codebase at the same time and make it harder to work on.

Also I noticed we are using a title prop for the title. I don't think that is a bad approach per se, but I do see some similarities between this component and the Modal for which we expose a ModalTitle. So you could say this is somewhat inconsistent. Not sure if we should change that....

Yeah true. I think that accepting a title prop is less hassle for the user of the component. It allows us more control over the styling of the title (placement for example), without forcing the user to go through the extra steps of importing a separate component, etc. Since we're moving away from enforcing certain components as children I think the prop also makes more sense, it's more react-like in my opinion.

Maybe before the release, we could adapt the other components, like the modal, to this pattern as well and unify it across the entire lib.

@ghost
ghost requested a review from HendrikThePendric April 8, 2020 12:21

@Mohammer5 Mohammer5 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks quite solid!
I don't really like the feature files' scenario titles, they're very technical instead of describing an actual scenario

@HendrikThePendric HendrikThePendric left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would rather move everything over at once than do it gradually for each PR, I feel like otherwise we'll have multiple conventions in the codebase at the same time and make it harder to work on.

Fair enough... I'm afraid we are already at the stage where we have these multiple conventions though. That started weeks ago....

@ghost
ghost force-pushed the add-noticebox branch from 7bffafa to d2cc48e Compare April 8, 2020 14:01
@ghost

ghost commented Apr 8, 2020

Copy link
Copy Markdown
Author

This looks quite solid!
I don't really like the feature files' scenario titles, they're very technical instead of describing an actual scenario

All right, updated it 👍

@ghost

ghost commented Apr 8, 2020

Copy link
Copy Markdown
Author

I would rather move everything over at once than do it gradually for each PR, I feel like otherwise we'll have multiple conventions in the codebase at the same time and make it harder to work on.

Fair enough... I'm afraid we are already at the stage where we have these multiple conventions though. That started weeks ago....

Yeah that's true. All right, I've updated it to the new format 👍

@ghost
ghost force-pushed the add-noticebox branch from f3a1b37 to 357ef6d Compare April 8, 2020 14:28
@ghost
ghost merged commit 5771a0e into alpha Apr 8, 2020
@ghost
ghost deleted the add-noticebox branch April 8, 2020 14:34
@dhis2-bot

Copy link
Copy Markdown
Contributor

@dhis2-bot

Copy link
Copy Markdown
Contributor

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants