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

Add HeroBanner #1125

Merged
merged 22 commits into from
Dec 2, 2017
Merged

Add HeroBanner #1125

merged 22 commits into from
Dec 2, 2017

Conversation

Sekhmet
Copy link
Contributor

@Sekhmet Sekhmet commented Nov 28, 2017

Fixes #999.

image

Changes:

  • Add HeroBanner component.
  • Support Affixes with dynamic content before sidebars.
  • Enable HeroBanner for unlogged users only.
  • Allow closing HeroBanner.
  • Style buttons.

@bonustrack bonustrack temporarily deployed to busy-master-pr-1125 November 28, 2017 23:01 Inactive
@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1125 November 29, 2017 17:17 Inactive
@Sekhmet Sekhmet removed the PR: WIP label Nov 29, 2017
@bonustrack
Copy link
Contributor

Wasn't supposed to be full-width, something like this:
image

I think we dont need separator between the buttons and we need bigger text and biggers buttons.

@jm90m
Copy link
Contributor

jm90m commented Nov 30, 2017

idk if this is related to this PR, but on for some reason everytime I try to login and authenticate, I get hit with this:

screen shot 2017-11-29 at 7 32 54 pm

}

&__content {
margin: auto 0;
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 you might be able to get rid of this if you are going to make the herobanner take up the entire screen width. And underneath &__content somewhere after around line 87 you can add the following flex properties:

align-items: center;
justify-contenter: center;

And then doing a few changes around like so:

in HeroBanner.js:
screen shot 2017-11-29 at 8 03 16 pm

in Page.js:
screen shot 2017-11-29 at 8 02 38 pm

in HeroBanner.less:
screen shot 2017-11-29 at 8 02 53 pm

Here's what I have in my local, after checking out your fork and branch, and testing it out.

screen shot 2017-11-29 at 7 56 55 pm

sry for the screenshots, but the git GUI that I use doesn't let me copy and paste lol.....

this.baseTopPosition = this.relativeContainer.getBoundingClientRect().top + this.lastScroll;

if (this.relativeContainer.parentElement) {
this.mo = new MutationObserver(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have never used MutationObserver before but I trust that this works 🤣

import { getIsAuthenticated, getIsLoaded, getIsBannerClosed } from '../reducers';
import { closeBanner } from '../app/appActions';

export default connect(
Copy link
Contributor

@jm90m jm90m Nov 30, 2017

Choose a reason for hiding this comment

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

I really like this idea, having a Container component that is a wrapper for the main component, and it handles all the state properties 👍

@@ -24,6 +24,7 @@
float: left;
display: none;
width: @left-width;
min-height: 1px;
Copy link
Contributor

Choose a reason for hiding this comment

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

won't need this anymore if banner is full width of the screen

@jm90m
Copy link
Contributor

jm90m commented Nov 30, 2017

had some small comments, but once @bonustrack approves it, its fine with me

@bonustrack bonustrack added this to the v2.2 milestone Dec 1, 2017
@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1125 December 1, 2017 13:22 Inactive
@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1125 December 1, 2017 13:32 Inactive
@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1125 December 1, 2017 13:43 Inactive
@bonustrack
Copy link
Contributor

Can we have bigger buttons? And can we center the content on small screen?

@bonustrack
Copy link
Contributor

The margin in top should not be bigger than margin in down
image

@Sekhmet Sekhmet temporarily deployed to busy-master-pr-1125 December 1, 2017 14:30 Inactive
@Sekhmet Sekhmet added the PR: WIP label Dec 1, 2017
Copy link
Contributor

@bonustrack bonustrack left a comment

Choose a reason for hiding this comment

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

👍

@Sekhmet Sekhmet merged commit e593ffd into busyorg:master Dec 2, 2017
@Sekhmet Sekhmet deleted the hero-banner branch December 2, 2017 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants