[ENG-6078][ENG-6080] Link to past reports #2296
Conversation
futa-ikeda
changed the title
| import Component from '@ember/component'; | ||
| import { action, computed } from '@ember/object'; | ||
| import { reads } from '@ember/object/computed'; | ||
| import Component from '@glimmer/component'; |
There was a problem hiding this comment.
You didn't mention in Changes that you were updating this to glimmer.
There was a problem hiding this comment.
Good call! I've updated the PR description!
futa-ikeda
merged commit 6028288
into
CenterForOpenScience:feature/insti-dash-improv
| .download-button { | ||
| display: inline-block; | ||
| border: 1px solid $color-border-gray; | ||
| border-radius: 2px; | ||
| padding: 6px 12px; | ||
| } | ||
|
|
||
| .download-button:focus, | ||
| .download-button:hover { | ||
| color: $color-text-black; | ||
| background-color: $color-shadow-dark; | ||
| border-color: $color-border-gray-darker; |
There was a problem hiding this comment.
(nitpick) is there a reusable component with style that applies here? (... i guess while Button has a "fake link" mode, OsfLink doesn't have a "fake button" mode... )
There was a problem hiding this comment.
Yea, this "OsfLink that looks like a button" is a pattern that we've used before. I guess I could just make this a Button that calls a window.open and that might be cleaner to maintain.
There was a problem hiding this comment.
<a href="..."> is preferable over a <button> with javascript to open a url (for both accessibility and clarity) -- if it's a common pattern to style a link as a button, tho, would recommend putting that consistent style in one place (whether OsfLink with corresponding arg, or in a reusable global css class or scss whatchamadoo) rather than rewriting it each time
There was a problem hiding this comment.
Re: <a> vs <button>, that's a good point. This is opening a URL after all.
I will add an arg to <OsfLink> similar to what we do for <Button>
| data-test-link-to-reports-archive | ||
| data-analytics-name='Link to archived reports' | ||
| local-class='download-button' | ||
| @href={{@institution.linkToExternalReportsArchive}} |
There was a problem hiding this comment.
(ux nitpick) as an external link, seems appropriate to open in a new tab/window with target="_blank" (unsure if there's a request either way on that)
| @@ -800,7 +800,7 @@ institutions: | |||
| preprints: Preprints | |||
| content-placeholder: Content coming soon # Delete this eventually pls | |||
| title: '{institutionName} Dashboard' | |||
| last_update: 'Updated every 24 hours' | |||
| download_past_reports_label: Reports | |||
There was a problem hiding this comment.
(text nitpick) seems less than ideal that the text shown to the user is even less descriptive than the variable names
There was a problem hiding this comment.
Yea, I'll propose a more descriptive label for this
…ForOpenScience/ember-osf-web into sort-users-for-inst-dashboard * 'feature/insti-dash-improv' of https://github.com/CenterForOpenScience/ember-osf-web: [ENG-6078][ENG-6080] Link to past reports (CenterForOpenScience#2296) # Conflicts: # app/institutions/dashboard/-components/institutional-users-list/component.ts
- Ticket: [] - Feature flag: n/a ## Purpose - Address some CR comments from prior PR #2296 ## Summary of Changes - Add a new arg `@fakeButton` to `OsfLink` to easily make a link look like a button (which maybe isn't a pattern we want to encourage, but at least now it's DRY when we do it 😬 ) - Update wording for hover text for reports - Have link open in a new window
Purpose
Summary of Changes
Screenshot(s)
Side Effects
QA Notes