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

Changed page-notice__cta to just use btn and btn--secondary #851

Merged
merged 3 commits into from Sep 24, 2019

Conversation

agliga
Copy link
Contributor

@agliga agliga commented Sep 23, 2019

Description

As per our discussion, changed the page-notice cta button to just use secondary button.
Changed styles for .btn and .fake-button to align with layout (in .page-notice)

Context

I had to force some layout changes, such as adding margin auto, or others in order to provide the different layouts for mweb and dweb.

Screenshots

image
image

@msendlakowski
Copy link
Contributor

Per discussion on Slack. We should update the browser.json for notice so it includes the button dependency.

@ianmcburnie
Copy link
Contributor

Looks good. Will leave it open until we see if Mike has any issues with this approach for success message.

notice.less Outdated Show resolved Hide resolved
@ianmcburnie
Copy link
Contributor

ianmcburnie commented Sep 24, 2019

@agliga Changes look good.

You're going to hate me, but I think I'm going to go with my original thought #452 (comment) and use a new block level class of .section-notice instead of page-notice--guidance. I just think even though right now the only difference is a border (which makes the css super simple), a section level notice is fundamentally different than a page level notice. It could deviate at any point to become more visually different than it is now (e.g. in the layout of its items), and if it does then a modifier could become tricky to work with. One other thing that sets it apart, is that it can host an overflow menu. This overflow menu might even necessitate that it becomes its own module at core-ui level. Don't worry though, I'll do the work this week in skin.

Anyhow, let's merge this one first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants