Skip to content

Conversation

@aaxelb
Copy link
Contributor

@aaxelb aaxelb commented Mar 16, 2018

Purpose

Use the {{link-to}} component for all internal links in ember-osf-web. Don't reload the page if we don't have to.

Summary of Changes

  • Extend link-to to accept ariaLabel and onClick arguments, to preserve existing behavior
  • Refactor navbar so it's not generating links from a list of objects, and some can be link-to while others are <a>
    • new-osf-navbar => osf-navbar
    • new-navbar-auth-dropdown => osf-navbar/auth-dropdown
    • build-secondary-nav-links helper replaced with osf-navbar/home-links component
    • Delete host-app-name mixin
    • Delete get-display-name helper
  • Typescriptize the things I touched

Side Effects / Testing Notes

  • Check that links to quickfiles, support, and the dashboard transition without reloading the page.
  • The navbar could use a regression test.

Ticket

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

Reviewer Checklist

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

@coveralls
Copy link

coveralls commented Mar 16, 2018

Coverage Status

Coverage decreased (-0.9%) to 21.122% when pulling 4f66854 on aaxelb:emb-167--link-to into 2f97e1b on CenterForOpenScience:develop.

});

actions = {
...this.actions, // from AnalyticsMixin
Copy link
Member

Choose a reason for hiding this comment

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

Nice! I didn't realize that this is possible.

@aaxelb aaxelb force-pushed the emb-167--link-to branch from 00620ad to 81ba72f Compare March 21, 2018 18:06
jamescdavis
jamescdavis previously approved these changes Mar 21, 2018
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.

Looks good once conflicts are resolved.

"bower_components"
"include": [
"app",
"tests"
Copy link
Member

Choose a reason for hiding this comment

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

TY! I was going to do this as a separate PR.


// TODO: When used in other apps, update to expect these as arguments or from the config
hostAppName: string = HOME_APP;
linksComponent: string = 'osf-navbar/home-links';
Copy link
Member

Choose a reason for hiding this comment

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

I like this approach much better than the one "link builder for all the apps" helper.

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.

👍

@jamescdavis jamescdavis merged commit cdd0fb2 into CenterForOpenScience:develop Mar 21, 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