Skip to content

Conversation

@hmoco
Copy link
Contributor

@hmoco hmoco commented Mar 5, 2018

Purpose

Support maintenance messages when they are needed.

Summary of Changes

Added maintenance-banner component, to display the maintenance banner when needed.

Also added support to use ember-wormhole for upcoming secondary navbars.

Ticket

https://openscience.atlassian.net/browse/EMB-153

Reviewer Checklist

  • meets requirements
  • easy to understand
  • DRY
  • testable and includes test(s)
  • changes described in CHANGELOG.md

Copy link
Member

@jamescdavis jamescdavis left a comment

Choose a reason for hiding this comment

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

Your test is failing...

Copy link
Contributor

@aaxelb aaxelb left a comment

Choose a reason for hiding this comment

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

The quickfiles navbar covers the maintenance banner (opacity manually squidgied):
screen shot 2018-03-06 at 5 17 33 pm

I think the right thing is probably taking this opportunity to improve the osf-navbar component. It could support a generic secondary navbar (used for quickfiles nav, project nav, anything else that comes up), and be in charge of pushing the rest of the page down as far as its height. That hardcoded offset should burn...

{{#if maintenance}}
<div class="scripted alert alert-dismissible {{alertClass}} MaintenanceBanner" role="alert">
<button type="button" class="close" data-dismiss="alert" aria-label="Close">
<span aria-hidden="true">{{fa-icon 'times'}}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

{{fa-icon}} already puts aria-hidden on for us

{{#if maintenance.message.length}}
{{maintenance.message}}
{{else}}
{{t 'maintenance.line1'}} <strong>{{start}} {{t 'general.and'}} {{end}}</strong> ({{utc}}).
Copy link
Contributor

Choose a reason for hiding this comment

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

We should generally avoid splitting up sentences like this. It assumes all translations will be structured like english, and makes the template harder for eyes to parse. ember-i18n has handlebars-style interpolation for just this sort of thing:

// translations.ts
maintenance: {
    line1: 'The site will undergo maintenance between <strong>{{start}} and {{end}}</strong> ({{utc}}).',
}
{{! maintenance-banner/template.hbs }}
{{t 'maintenance.line1' start=start end=end utc=utc}}

@hmoco
Copy link
Contributor Author

hmoco commented Mar 7, 2018

Unfortunately the banner needs to go underneath the secondary navigation, and we needs to be ablet to scroll away from it, which prevents us from being able to add it to the navbar
screen shot 2018-03-07 at 2 27 02 pm

@coveralls
Copy link

coveralls commented Mar 7, 2018

Coverage Status

Coverage remained the same at 29.397% when pulling cabaa4d on hmoco:feature/maintenance into b04de90 on CenterForOpenScience:develop.

@aaxelb
Copy link
Contributor

aaxelb commented Mar 7, 2018

I think one component could be in charge of both rendering the navbar and claiming the space behind it. It'd be easier to reason about if it were split into subcomponents, maybe like:

{{! components/osf-navbar/template.hbs }}
{{osf-navbar/primary-nav}} {{! fixed to the top, basically the old `new-osf-navbar` component }}
<div class='PrimaryBuffer'></div>

{{#if secondaryLinks}}
    {{osf-navbar/secondary-nav links=secondaryLinks}} {{! fixed under the primary nav }}
    <div class='SecondaryBuffer'></div>
{{/if}}

{{! everything below (the whole app) will be pushed down by the buffer divs }}

{{osf-navbar/maintenance-banner}} {{! could also be invoked in `application/template.hbs`, doesn't matter }}

(I imagine secondaryLinks could live on a service, maybe? so each route could decide its navbar situation and update the service in activate/deactivate)

// styles/_variables.scss
$navbar-primary-height = 50px;
$navbar-secondary-height = 50px;


// components/osf-navbar/styles.scss
.PrimaryBuffer {
  height: $navbar-primary-height;
}

.SecondaryBuffer {
  height: $navbar-secondary-height;
}


// components/osf-navbar/primary-nav/styles.scss
.NavBar {
  height: $navbar-primary-height;
}


// components/osf-navbar/secondary-nav/styles.scss
.NavBar {
  height: $navbar-secondary-height;
}

I don't know; what do you think?

@hmoco
Copy link
Contributor Author

hmoco commented Mar 7, 2018

@aaxelb, I really like the idea. My one concern with it is, unless I'm misunderstanding it, that we would need to generalize the secondary navbar, allowing for (when we do the contributors page) different secondary navbars. For example, the project page would need to eventually allow for the comments panel. So, instead of passing down the secondaryLinks, it would make more sense to pass down a component representing the secondary navbar. We've definitely done that before, with preprints, where the generalized discover page took components as arguments to render different filters within the discover component. So, possible, but might be a little more complex than a secondaryLinks service implies. Thoughts?

@aaxelb
Copy link
Contributor

aaxelb commented Mar 7, 2018

@hmoco Hmm, good point. I still like the idea of a navbar service, but passing it a component name would be way cleaner than the list of objects and dynamically generated links I was imagining...

Something like this?

// services/navbar.ts
setNavbarState(secondaryNavComponent, secondaryNavContext) { // but not those names, something better
  this.setProperties({ secondaryNavComponent, secondaryNavContext });
}

clearNavbarState() { ... }
{{! components/osf-navbar/template.hbs }}
{{component navbar.secondaryNavComponent context=navbar.secondaryNavContext}}
// guid-user/quickfiles/route.ts
activate() {
  this.get('navbar').setNavbarState('quickfiles-nav', this.modelFor('guid-user'));
}
deactivate() {
  this.get('navbar').clearNavbarState();
}
// guid-node/route.ts
activate() {
  this.get('navbar').setNavbarState('project-nav', this.modelFor('guid-node'));
}
// guid-file/route.ts
activate() {
  // might be some trickery if the file hasn't loaded yet... 
  // could do it in the model task instead? or pass the task itself?
  if (isQuickfile) { 
    this.get('navbar').setNavbarState('quickfiles-nav', ...);
  } else {
    this.get('navbar').setNavbarState('project-nav', ...);
  }
}

@hmoco
Copy link
Contributor Author

hmoco commented Mar 8, 2018

I'm not convinced that adding secondary navbar logic to the main navbar is worth dealing with, as you've shown in your examples, activate/deactivate logic in the route, and dealing with model states. A static check in the component scope seems simpler, although I agree that the maintenance banner should not be statically included in every template (the secondary navbar tradeoff is including relevant navbars on each template that uses it or delegate so that each route passes it to the primary navbar). Will look into solutions for adding the maintenance bar. Let me know if you have further thoughts or want to sit down and find a solution

Copy link
Member

@jamescdavis jamescdavis left a comment

Choose a reason for hiding this comment

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

Please resolve conflict. (see: #111)

@jamescdavis
Copy link
Member

@jamescdavis jamescdavis merged commit ce82df1 into CenterForOpenScience:develop Mar 9, 2018
@jamescdavis jamescdavis added this to the 0.3.0 milestone May 7, 2019
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