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.

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

Feat: Update Mission Modal Blueprint w/ new (WIP) Rating component #1813

Merged
merged 9 commits into from
Apr 8, 2020

Conversation

sghoweri
Copy link
Contributor

@sghoweri sghoweri commented Apr 7, 2020

Jira

http://vjira2:8080/browse/BDS-2089

Summary

Updates the existing Mission Modal blueprint with updated design comps; new Rating component.

image

Details

  • Includes a fix for a tiny z-index related issue I ran into while using the new Card component's background layer.
  • Also includes some demo JS to show the submit button requiring a rating to be chosen.

How to test

  • Review updated Modal blueprints (before and after submitting) -- any large red flags or updates needed in order to get this out the door?

@sghoweri sghoweri added this to the v2.21.0 milestone Apr 7, 2020
@sghoweri sghoweri requested a deployment to dev April 7, 2020 14:42 Abandoned
@sghoweri sghoweri temporarily deployed to dev April 7, 2020 14:53 Inactive
@@ -11,10 +11,17 @@ bolt-card-replacement-background {
right: 0;
bottom: 0;
left: 0;
z-index: 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sghoweri this is going to break the z-index management in here. Instead, can you try this? I have this working locally:

bolt-card-replacement-background {
  position: absolute;
  top: 0;
  right: 0;
  bottom: 0;
  left: 0;
  z-index: $bolt-card-replacement-z-index-background;
  overflow: hidden;
  pointer-events: none;
  user-select: none;
  border-radius: $bolt-card-replacement-border-radius;

  [rounded] & {
    @include bolt-border-radius(large);
  }

  & + bolt-card-replacement-body {
    margin-bottom: auto;
  }
}

.c-bolt-card_replacement__background {
  position: relative;
  width: 100%;
  height: 100%;
}

z-index needs to be set on the element that's controlling the stacking order, the children will follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikemai2awesome unfortunately this update alone isn't enough to fix the main issue I was running into:

image

That said, I'm testing out one additional CSS update to have the immediate children of <bolt-card-replacement> automatically get position: relative which fixes the issue... just testing if there's any gotchas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikemai2awesome check out the latest I just pushed up!

Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

@sghoweri Nice job putting this together so quickly. I added some inline comments/questions. Let me know what you think.

Also, the background image isn't visible for me locally or on feature branch: https://feature-update-mission-modal.boltdesignsystem.com/pattern-lab/?p=blueprints-t1-mission-landing--test-with-modal

@sghoweri
Copy link
Contributor Author

sghoweri commented Apr 7, 2020

@sghoweri Nice job putting this together so quickly. I added some inline comments/questions. Let me know what you think.

Also, the background image isn't visible for me locally or on feature branch: https://feature-update-mission-modal.boltdesignsystem.com/pattern-lab/?p=blueprints-t1-mission-landing--test-with-modal

@danielamorse I should have a fix for that pushed up very shortly -- the image is there... it's just not showing up due to the related updates I pushed up to address @mikemai2awesome's feedback.



// WIP Rating Component
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking ahead, how do you see this SASS being added to Academy? So far there's only vanilla CSS. Do you know if SASS is supported at the theme level? If not, we'll find out together :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danielamorse this Sass is already getting pulled into Academy (by them installing and using Blueprints) -- if anything, looking ahead I see this Sass getting moved out of here and into a Rating-specific Component package.

@sghoweri sghoweri temporarily deployed to commit-preview April 8, 2020 14:51 Inactive
@sghoweri sghoweri temporarily deployed to branch-preview April 8, 2020 14:58 Inactive
@sghoweri sghoweri temporarily deployed to commit-preview April 8, 2020 15:31 Inactive
@sghoweri sghoweri temporarily deployed to branch-preview April 8, 2020 15:37 Inactive
@sghoweri sghoweri temporarily deployed to commit-preview April 8, 2020 16:45 Inactive
@mikemai2awesome
Copy link
Collaborator

mikemai2awesome commented Apr 8, 2020

@sghoweri @danielamorse

I took care of my own comments above with the latest commit, plus the following:

  1. Made sure the white part of the background image does not crash with white text at any viewport.
  2. Made design judgements on a few utility classes and removed them. We shouldn't be overriding so much for minimal gains.

@sghoweri sghoweri temporarily deployed to branch-preview April 8, 2020 16:53 Inactive
@danielamorse danielamorse self-requested a review April 8, 2020 17:15
@danielamorse danielamorse merged commit 7f4ad7d into master Apr 8, 2020
@danielamorse danielamorse deleted the feature/update-mission-modal branch April 8, 2020 17:16
@sghoweri
Copy link
Contributor Author

sghoweri commented Apr 8, 2020

PR was released with v2.21.0

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

3 participants