Skip to content

Center banner text#4171

Merged
slj merged 4 commits intomasterfrom
slj-center-banner-text
Sep 9, 2020
Merged

Center banner text#4171
slj merged 4 commits intomasterfrom
slj-center-banner-text

Conversation

@slj
Copy link
Contributor

@slj slj commented Sep 9, 2020

Why: LG-3104
How: CSS tweaks
How: Redo banner close button (mobile) w/html + js

**Why**: LG-3104
**How**: CSS tweaks
**How**: Redo banner close button (mobile) w/html + js
@slj
Copy link
Contributor Author

slj commented Sep 9, 2020

Desktop view evident from Preview. Mobile view before:
mobile-ctr-banner-before

Mobile view after:
mobile-ctr-banner-after

</div>
</div>
</div>
<button class="mobile close-banner-how-you-know" aria-label="close banner popup"></button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to translate the aria-label?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh...good call. Going too fast. Thx.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@slj slj merged commit 697dd56 into master Sep 9, 2020
@slj slj deleted the slj-center-banner-text branch September 9, 2020 20:59
}

.close-banner-how-you-know {
background: url(#{$image-path}/close-primary.svg) top left no-repeat;
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed a console error in unrelated testing in the development environment, I suspect is caused by this.

[Error] Failed to load resource: the server responded with a status of 403 () (close-primary.svg, line 0)

Based on how images are referenced in other stylesheets, I suspect this should be image-path helper?

Suggested change
background: url(#{$image-path}/close-primary.svg) top left no-repeat;
background: url(image-path('close-primary.svg')) top left no-repeat;

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried this, but it doesn't fix it. Does the image exist?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's in the identity-style-guide:

identity-site:master> find node_modules -name '*close-primary.svg' 
node_modules/identity-style-guide/dist/assets/img/close-primary.svg

Copy link
Contributor Author

@slj slj Sep 14, 2020

Choose a reason for hiding this comment

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

shucks...it worked locally, but clearly something is amiss...easy fix would be to copy node_modules/identity-style-guide/dist/assets/img/close-primary.svg into the path referenced by the image-path helper, but I'd still be left wondering how the dist assets are supposed to be reached...

In any event, good catch @mitchellhenke

Copy link
Contributor

Choose a reason for hiding this comment

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

I get GET http://localhost:3000/images/close-primary.svg 404 (Not Found) with it locally

Copy link
Contributor

@zachmargolis zachmargolis Sep 14, 2020

Choose a reason for hiding this comment

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

It gets copied to:

identity-site:master> find _site -name '*close-primary.svg'
_site/assets/img/close-primary.svg

EDIT: whoops, my brain is fried, this is the wrong repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After git pulling on master, I can't get the project to run anymore. Seems like the webpack build is not quite working...stay tuned...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Continually getting this:
Screen Shot 2020-09-14 at 12 07 34 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#4171 may be the solution. Draft PR built successfully.

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.

4 participants