Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Build Beta banner component and modal #266

Merged
merged 12 commits into from
Nov 21, 2022
Merged

Build Beta banner component and modal #266

merged 12 commits into from
Nov 21, 2022

Conversation

create-issue-branch[bot]
Copy link
Contributor

@create-issue-branch create-issue-branch bot commented Nov 14, 2022 •
edited by loiswells97
Loading

What's Changed?

  • Added beta banner with link to open modal explaining more details about what beta means

Screenshots

Banner

Screenshot 2022-11-16 at 09 35 36

Modal

Screenshot 2022-11-16 at 09 36 23

closes #256

@create-issue-branch create-issue-branch bot temporarily deployed to previews/issues/256-Build_Beta_banner_component_and_modal November 14, 2022 12:44 Inactive
@github-actions
Copy link

@github-actions
Copy link

@loiswells97 loiswells97 temporarily deployed to previews/issues/256-Build_Beta_banner_component_and_modal November 14, 2022 16:58 Inactive
@github-actions
Copy link

@loiswells97 loiswells97 temporarily deployed to previews/issues/256-Build_Beta_banner_component_and_modal November 14, 2022 17:28 Inactive
@github-actions
Copy link

@loiswells97 loiswells97 temporarily deployed to previews/issues/256-Build_Beta_banner_component_and_modal November 14, 2022 17:32 Inactive
@github-actions
Copy link

@loiswells97 loiswells97 temporarily deployed to previews/issues/256-Build_Beta_banner_component_and_modal November 15, 2022 11:00 Inactive
@github-actions
Copy link

@loiswells97 loiswells97 temporarily deployed to previews/issues/256-Build_Beta_banner_component_and_modal November 15, 2022 17:28 Inactive
@github-actions
Copy link

@loiswells97 loiswells97 temporarily deployed to previews/issues/256-Build_Beta_banner_component_and_modal November 15, 2022 17:39 Inactive
@github-actions
Copy link

@loiswells97 loiswells97 temporarily deployed to previews/issues/256-Build_Beta_banner_component_and_modal November 16, 2022 09:31 Inactive
@loiswells97 loiswells97 marked this pull request as ready for review November 16, 2022 09:31
@github-actions
Copy link

@maxelkins
Copy link
Contributor

maxelkins commented Nov 16, 2022 •
edited
Loading

Design Review

Banner looks and works great :)

With the modal I think we may need to tweak how its sizing works. To make it more responsive
image

Some possible suggestions for intended behavior:

  • On the modal overlay
display: flex;
justify-content: center;
align-items: center;
  • On the modal
    image
    • Ofc feel free to swap the 32px for appropriate variable.

Other thoughts

  • We should probably think about how scrolling may work in a modal if needed, as currently if it is larger than the screen height content will be cut off. For this case we should be okay if above changes are made :)

@loiswells97 loiswells97 temporarily deployed to previews/issues/256-Build_Beta_banner_component_and_modal November 16, 2022 14:07 Inactive
@github-actions
Copy link

@loiswells97 loiswells97 temporarily deployed to previews/issues/256-Build_Beta_banner_component_and_modal November 17, 2022 15:18 Inactive
@github-actions
Copy link

sHtev
sHtev suggested changes Nov 18, 2022
View reviewed changes
Copy link
Contributor

@sHtev sHtev left a comment

Choose a reason for hiding this comment

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

Some comments on CSS organisation when using BEM

src/Modal.scss Outdated
@@ -18,22 +18,34 @@

.modal__header {
Copy link
Contributor

Choose a reason for hiding this comment

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

you shouldn't need these indented in modal__content, as they should stand alone (in BEM)

src/Modal.scss Outdated
}
}

label {
.modal__heading {
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

src/Modal.scss Outdated
font-size: var(--font-size-u-2)
}

.modal__subheading {
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

src/Modal.scss Outdated
font-weight: var(--font-weight-bold);
}

.modal__text {
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

src/components/BetaBanner/BetaBanner.scss Outdated
align-items: center;
padding: var(--spacing-half) var(--spacing-2);

&__message {
Copy link
Contributor

Choose a reason for hiding this comment

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

for elements, they should be stand-alone, see modal

src/components/BetaBanner/BetaBanner.scss Outdated
padding: var(--spacing-half) 0
}

&__link {
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

src/components/BetaBanner/BetaBanner.scss Outdated
text-decoration: underline;
}

&__close-button {
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

src/components/BetaBanner/BetaBanner.scss Outdated
background-color: $editor-turquoise-tint-50;
color: $editor-black;

&__icon {
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

@loiswells97 loiswells97 temporarily deployed to previews/issues/256-Build_Beta_banner_component_and_modal November 21, 2022 16:31 Inactive
@github-actions
Copy link

@github-actions
Copy link

@sHtev sHtev merged commit 0ac1f25 into main Nov 21, 2022
@sHtev sHtev deleted the issues/256-Build_Beta_banner_component_and_modal branch November 21, 2022 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sHtev sHtev sHtev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Build Beta banner component and modal
4 participants
@maxelkins @sHtev @loiswells97 @maddyjoe