Skip to content

banner updates #143

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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

banner updates #143

wants to merge 19 commits into from

Conversation

kimispencer
Copy link
Contributor

@kimispencer kimispencer commented Apr 21, 2020

No description provided.

arthurxavierx and others added 7 commits December 12, 2019 10:47

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@kimispencer kimispencer self-assigned this Apr 21, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@kimispencer kimispencer changed the base branch from arthur/banner to master May 2, 2020 11:06

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@kimispencer kimispencer requested a review from arthurxavierx May 12, 2020 11:26
@kimispencer kimispencer changed the title [WIP] banner updates banner updates May 12, 2020
@kimispencer kimispencer requested a review from maddie927 May 12, 2020 11:27
Copy link
Contributor

@arthurxavierx arthurxavierx left a comment

Choose a reason for hiding this comment

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

This is looking pretty good!!

-- @NOTE positioning of the action button changes dependent on if there is a banner title or not
{ alignItems: if (isJust props.title)
then str "flex-end"
else str "center"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this style should go in the box that contains only the actions, otherwise the banner content also gets right-aligned on mobile:
Screen Shot 2020-05-12 at 09 59 30

Copy link
Contributor

@arthurxavierx arthurxavierx May 12, 2020

Choose a reason for hiding this comment

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

Also, from looking at the design documentation, it seems that it's the presence of an icon that affects the positioning of the action buttons.

Anyway, I still think we don't need this special casing though, as it looks weird also from a UX perspective to have the buttons change side when an icon exists; that pattern change might confuse users. I'm discussing that with Bethany in the design document.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
$ Banner.actionBanner twoButtonActions
$ _ { content = loremIpsum_ }

, h2_ "action banner, round (default), spaced list, non-dismissable"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we also want an example/test with actions and the dismiss button.

, dismissable :: Boolean
, icon :: Maybe JSX
, title :: Maybe JSX
, content :: Array JSX
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I've noticed just now is that, with these types, we don't let the user insert content in the banner that may dismiss it, including the actions. We cannot have now an action button that dismisses the banner.

Maybe we should change the type of content to Effect Unit -> Array JSX? cc @spicydonuts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah that's a really good point...

Also relates to your above comment (re an example with actions + the dismiss button) from what I could tell in the designs it's an either/or -- that is to say, either a banner has text + a dismiss X or has text + actions (and on those action button presses the banner will also be dismissed on completion of the action).

@maddie927 maddie927 changed the base branch from master to main June 22, 2020 19:29
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.

None yet

2 participants