Feature/New OSF Navbar [EOSF-511] #188

pattisdr · 2017-02-28T22:38:31Z

Ticket

https://openscience.atlassian.net/browse/EOSF-511

Companion PR ❗️ Styles have been moved to osf-style guide CenterForOpenScience/osf-style#95

Purpose

Ember-OSF component that is a redesign of the navbar. For use in preprints and registries. Ensures that navigation within services is easy and clear without needing two stacked navigation bars, so that visitors to the site can easily orient themselves.

Screenshots

Home primary navigation selected

Preprints primary navigation selected

Service dropdown

Auth dropdown

…tings.

…is navigation will not collapse on xs screen. Should behave normally.

…nabber.

…opdown.

…avbar-auth-dropdown opened.

…d of a unit test.

…er-osf into feature/new-osf-navbar

binoculars · 2017-03-23T15:41:03Z

addon/components/new-osf-navbar/style.scss

+    /* Overrides color of dropdown list hover (applies to both service and auth dropdowns) */
+    .dropdown-menu > li > a:hover,
+    .dropdown-menu > li > a:focus {
+          background-image: -webkit-linear-gradient(top, $osf-highlight-navbar 0%, $osf-highlight-navbar 100%);


There's an ember-cli addon that helps with this. The benefit is that over time, we can eliminate the prefixes for unsupported browsers and not have to think about it.

Simplify CSS. Get rid of teal background. Inject session into helper rather than passing it in. Tweak styles. Turn search link into search button that looks like a link. Use session.get(...). Correct home service link.

…er-osf into feature/new-osf-navbar # Conflicts: # addon/components/osf-footer/template.hbs

…ber-osf into feature/new-osf-navbar

… new page instead.

abought

A few small notes and questions in this round, but overall positive steps forward; this is looking really nice. The updated design for the service selector feels cleaner and makes good use of space; it also seems to work nicely on mobile.

Pending your triage of a few notes, I wouldn't necessarily need to return to this in another round of review myself, so long as we expect QA will have access to the new navbar for an extended round of functional review.

abought · 2017-04-20T18:45:02Z

addon/components/navbar-auth-dropdown/component.js

    signupUrl: config.OSF.url + 'register',

+    serviceLinks: Ember.computed(function() {
+        return serviceLinks;


What is the advantage of using a computed vs setting this as the property value directly? (serviceLinks: serviceLinks)

abought · 2017-04-20T18:49:51Z

addon/components/osf-navbar/component.js

-    hideSearch: false,
-
+    osfServices: Ember.computed(function() {
+        return osfServices;


Lines 30 and 33 here, similar to above: what is the advantage of using a computed here instead of setting the value of the property directly?

abought · 2017-04-20T18:53:40Z

addon/components/osf-navbar/component.js

+    }),
    host: config.OSF.url,
-
+    currentService: Ember.computed(function() { // Pulls current service name from consuming service's config file


This computed relies on the property hostAppName; it's good practice for a computed to specify (and watch) the values it depends on.

In general I don't expect this value to change while the app is loaded- but it's possible ember apps could merge in the future. Recommend the change as some cheap, easy future proofing.

Good point about the ember apps merging.

abought · 2017-04-20T18:55:20Z

addon/components/osf-navbar/component.js

+            this.send('closeSecondaryNavigation');
        },
+        closeSecondaryNavigation() {
+            Ember.$('.navbar-collapse').collapse('hide');


This appears to refer to an element within the same component. For performance and to make the selector more specific, consider usingthis.$() instead.

abought · 2017-04-20T19:24:18Z

bower.json

  "devDependencies": {
    "bootstrap": "~3.3.5",
-    "osf-style": "https://github.com/CenterForOpenScience/osf-style.git#48197d234baf07c321c56edfd9f48b5624374453"
+    "osf-style": "https://github.com/pattisdr/osf-style.git#32b2584d0087db6895645a5c58518861cbe7d004"


Reminder to final reviewers; this will need to be changed before merge.

abought · 2017-04-20T19:40:57Z

addon/const/osf-services.js

+    }
+];
+
+export default osfServices;


Since this is so tightly connected to serviceLinks, would it make sense to combine osf-services and service-links in the same file, as a logically related grouping? Remember that a single module can export more than one thing.

While I realize this seems pedantic, all ember-osf PRs get reviewed under the assumption that we don't control addon users, and therefore breaking changes become much harder to coordinate once a feature is merged.

~~I don't think they should be combined. osf-services is mainly going to be used on this new navbar.~~ Thinking about it more, I will combine them.

abought · 2017-04-20T19:47:41Z

tests/integration/helpers/build-secondary-nav-links-test.js

+import tHelper from "ember-i18n/helper";
+
+// Stub i18n service
+const i18nStub = Ember.Service.extend({


Very nice use of stubbed services here!

pattisdr · 2017-04-20T23:03:18Z

Changes made @abought.

pattisdr · 2017-05-02T10:56:06Z

Closing in favor of #204

pattisdr added 30 commits February 24, 2017 09:16

Start new-osf-navbar component.

45fc8ba

Add Novecento Sans fonts. Should probably be moved to osf-style.

89d380e

Add collapsible navbar with links to Home, Preprints, Registries, Mee…

a74c8d8

…tings.

Loop through OSF services to display in navbar.

06ee8ff

Try to add service bar [WIP].

6fd4e6c

Add formatting for the secondary navbar on all screens but xs.

2c3ab9f

Removes nav-navbar and nav classes from primary navigation because th…

8748fa1

…is navigation will not collapse on xs screen. Should behave normally.

Responsive changes for teal secondary navbar.

7f80e07

Add more granularity to media queries for resizing of teal secondary …

e317b71

…nabber.

Add helper to generate secondary navigation links.

2b906fc

Make secondary navigation links and user links collapsible.

d60714d

Add registrations help link.

06c551f

Pass in loginAction.

96001e0

Modify position of sign in/ login buttons.

a1e69f0

Adjust widths of secondary navbar.

853384f

Revert defining loginAction on component. Will pass in.

4836d0f

Fix some js/template issues.

4e2bf57

Add some tests.

9afcd0c

Add different dropdown formatting for service dropdown and profile dr…

3d9f957

…opdown.

Increase height of navbar to original height.

b515008

Removes css triangle on xs screens for profile dropdown.

19c2650

Change display of sign in / sign up buttons.

abf9b6d

Change positioning of secondary navigation links on xs screen.

f507a27

Overrides positioning of signup/signin buttons on xs screen.

93dc84a

Adjust height of secondary links.

883601e

Display search dropdown when clicking on 'Search' in OSF nabber.

45ca889

Pass in currentService to determine which secondary links are displayed.

7627fa0

Close secondary navigation when primary navigation is opened.

e221b7d

Close search if secondary navigation opened.

06fbae8

Pass in action to navbar-auth-dropdown to close other navigation if n…

7d66dfb

…avbar-auth-dropdown opened.

pattisdr added 9 commits March 3, 2017 13:23

Test build-secondary-nav-links helper with an integration test instea…

3326737

…d of a unit test.

Add search help modal.

66aae70

Add close translation.

f21bc85

Styling tweaks.

74b5979

Turn serviceName into a link.

a87c78f

Add currentServiceLink.

e79fa81

Modify styles and make navbar fixed.

1296c80

JS error.

c993480

Merge branch 'develop' of https://github.com/CenterForOpenScience/emb…

a282df5

…er-osf into feature/new-osf-navbar

This was referenced Mar 3, 2017

Feature/New Navbar in Registries[EOSF-543] CenterForOpenScience/ember-osf-registries#9

Merged

Feature/Use New Navbar in Preprints [EOSF-541] CenterForOpenScience/ember-osf-preprints#294

Merged

Increase top margin of secondary nav bar.

8d88fb4

binoculars reviewed Mar 23, 2017

View reviewed changes

pattisdr added 11 commits April 3, 2017 14:02

Refactor navbar to accommodate user feedback.

e689643

Simplify CSS. Get rid of teal background. Inject session into helper rather than passing it in. Tweak styles. Turn search link into search button that looks like a link. Use session.get(...). Correct home service link.

Merge branch 'develop' of https://github.com/CenterForOpenScience/emb…

713e448

…er-osf into feature/new-osf-navbar # Conflicts: # addon/components/osf-footer/template.hbs

Add cursor:pointer to logout link.

b065d58

Restore service link.

14693a3

Move new navbar styles to style guide.

09e5e2a

Point to temporary osf-style branch.

6cc0aa6

`Merge branch 'develop' of https://github.com/CenterForOpenScience/em…

6e1025f

…ber-osf into feature/new-osf-navbar

Update osf-style commit.

3f0f85e

Do not switch service links when toggle between services. Navigate to…

e76fff1

… new page instead.

Use hostAppName instead of currentService.

a40ff80

Remove space.

b3eb70a

abought reviewed Apr 20, 2017

View reviewed changes

Code review changes.

9aab36e

Consolidate serviceLinks and osfServices into the same file.

9539f50

pattisdr closed this May 2, 2017

Feature/New OSF Navbar [EOSF-511] #188

Feature/New OSF Navbar [EOSF-511] #188

Uh oh!

Conversation

pattisdr commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

Purpose

Screenshots

Home primary navigation selected

Preprints primary navigation selected

Service dropdown

Auth dropdown

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abought left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pattisdr Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pattisdr commented Apr 20, 2017

Uh oh!

pattisdr commented May 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pattisdr commented Feb 28, 2017 •

edited

Loading

abought left a comment •

edited

Loading

pattisdr Apr 20, 2017 •

edited

Loading