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

fix sticky button bug #577

Merged
merged 2 commits into from
Nov 17, 2021
Merged

fix sticky button bug #577

merged 2 commits into from
Nov 17, 2021

Conversation

smcalilly
Copy link
Contributor

@smcalilly smcalilly commented Nov 17, 2021

Overview

Closes issue #531.

Demo

sticky-top

Notes

The existing sticky-top CSS was declared in two places: bootstrap-overrides.css and _position.css. I changed it in bootstrap-overrides.css and this worked. I wasn't sure about the purpose of _position.css file since the existing sticky-top CSS declaration was already set elsewhere, so I added it there just to be safe.

Testing

  • Pull down sticky-button branch
  • Visit a department and unity page
  • Make sure the button looks right

@smcalilly smcalilly requested review from fgregg and hancush November 17, 2021 15:39
@fgregg
Copy link
Contributor

fgregg commented Nov 17, 2021

@hancush could you speak to whether we need this code in _position.cc?

Comment on lines 39 to 45
.sticky-top-button {
@supports (position: sticky) {
position: sticky;
top: 55px;
z-index: $zindex-sticky;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

The static/bootstrap/* Sass files define a custom Bootstrap build, which the bootstrap-overrides.css file then, as you might guess, overrides. (Docs would be too convenient, but I took and documented a similar approach over in CA FWD: https://github.com/datamade/california-dream-index#compiling-sass-to-css)

So, you only need to define this new custom class in bootstrap-overrides.css.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, the most appropriate place would probably be custom.css.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that makes sense x 2. changed here d41957b. @fgregg is this ready to merge otherwise?

@fgregg fgregg merged commit 00f02f0 into master Nov 17, 2021
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.

3 participants