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

[ENG-4418] No BsAlert #1861

Conversation

brianjgeiger
Copy link
Contributor

@brianjgeiger brianjgeiger commented May 9, 2023

Purpose

Remove usage of the BsAlert from ember osf web.

Summary of Changes

  1. Remove handbook docs
  2. Stop subclassing BsAlert and just call it Alert
  3. Fix View Only Link Banner
  4. Fix Maintenance Banner
  5. Fix Status Banners
  6. Fix TOS Consent Banner
  7. Fix tests

Screenshot(s)

Before
Screenshot 2023-05-05 at 10 26 40 AM

After
Screenshot 2023-05-09 at 12 42 37 PM

Side Effects

  1. Close buttons on the status banner are larger, and the banners are a little smaller. I could fix this, but I kind of like it.
  2. You now have to explicitly pass in @visible={{true}} or similar to the Alert component to get it to show.

QA Notes

Check to make sure that the various alert banners work.

  • View Only Link Banner
  • Maintenance Banner
  • Status Banners
  • TOS Consent Banner

@@ -67,6 +67,6 @@ export default class TosConsentBanner extends Component {
this.analytics.click('button', 'ToS Consent Banner - dismiss');
this.set('didValidate', false);
this.currentUser.set('showTosConsentBanner', false);
return false;
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a return value here in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used by the dismiss action in the component.

@brianjgeiger brianjgeiger merged commit f7fcaae into CenterForOpenScience:basket/cesium May 11, 2023
@brianjgeiger brianjgeiger deleted the cesium/no-bs-alert branch May 11, 2023 19:02
@brianjgeiger brianjgeiger added this to the 23.08.0 milestone Jul 13, 2023
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