[REG-13] Registries fixes by chrisseto · Pull Request #415 · CenterForOpenScience/ember-osf-web

chrisseto · 2018-10-03T18:39:20Z

Purpose

Fixes for Registries from QA.

Summary of Changes

See Commits

Side Effects

Feature Flags

QA Notes

Ticket

https://openscience.atlassian.net/browse/REG-13

Reviewer Checklist

aaxelb

Overall looks good, just a few things

aaxelb · 2018-10-04T13:07:10Z

lib/osf-components/addon/components/search-help-modal/template.hbs

@@ -0,0 +1,25 @@
+{{! template-lint-disable bare-strings }}


I don't see any bare strings?

aaxelb · 2018-10-04T13:24:19Z

lib/registries/addon/helpers/math.ts

+        } else if (content[i] === '}') {
+            contextLevel++;
+        } else if (content[i] === '{') {
+            contextLevel--;


If I understand the intent, I think you got these flipped. When it finds a {, should be contextLevel++ to prevent matching an end delimiter inside brackets.

just text $$ math! { $$ math } still math $$ okay text again

aaxelb · 2018-10-04T13:30:49Z

lib/registries/addon/helpers/math.ts

+export const DELIMITERS: Delimiter[] = [
+    { start: '$$', end: '$$', inline: false },
+    { start: '\\[', end: '\\]', inline: false },
+    { start: '$', end: '$', inline: true },


This one seems easy to hit accidentally, like "The GDP of Lemonadia increased from $7 all the way to $12 in one week." Does normal text rendered as math look weird?

It will look weird but I don't actually think it will be that common to hit. However using $ expr $ seem to be fairly standard?

Currently MathJax will mess it up as well and we're explicitly opting into that behavior.

Hm, would it be reasonable to put spaces in the delimiter? Maybe start: ' $ ', end: ' $ ' or start: '$ ', end: ' $'? I guess it feels like using a dollar symbol twice in one title or description might happen pretty often in some fields.

aaxelb · 2018-10-04T13:36:15Z

tests/engines/registries/unit/helpers/math-test.ts

+    (expr: string) => `This is TeX: ${expr}`,
+    (expr: string) => `${expr} is some TeX`,
+    (expr: string) => `${expr} TeX is leading and ending here ${expr}`,
+    (expr: string) => `${expr} ${expr} Double Trouble!`,


maybe a Some TeX ${expr} surrounded by text?

aaxelb · 2018-10-04T13:45:38Z

lib/osf-components/addon/components/search-help-modal/component.ts

@@ -0,0 +1,40 @@
+import { layout } from '@ember-decorators/component';


looks like there's already a search-help-modal in app-components, seems like they should be united

coveralls · 2018-10-04T18:52:06Z

Pull Request Test Coverage Report for Build 1663

20 of 24 (83.33%) changed or added relevant lines in 8 files are covered.
15 unchanged lines in 6 files lost coverage.
Overall coverage increased (+0.2%) to 62.631%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/collections/addon/discover/controller.ts	0	1	0.0%
lib/registries/addon/components/registries-search-result/component.ts	5	6	83.33%
lib/registries/addon/services/share-search.ts	0	2	0.0%

Files with Coverage Reduction	New Missed Lines	%
lib/osf-components/addon/components/osf-navbar/component.ts	1	79.37%
lib/registries/addon/components/registries-search-result/component.ts	1	83.64%
lib/osf-components/addon/components/osf-navbar/auth-dropdown/component.ts	2	86.57%
lib/registries/addon/discover/controller.ts	3	91.28%
lib/registries/addon/services/share-search.ts	4	40.21%
lib/registries/addon/services/search.ts	4	70.0%

Totals
Change from base Build 1661:	0.2%
Covered Lines:	3578
Relevant Lines:	5446

💛 - Coveralls

jamescdavis

Several questions, couple of nits

jamescdavis · 2018-10-05T13:31:57Z

app/locales/en/translations.ts

                refine_your_search: 'Refine your search by',
                clear_filters: 'Clear Filters',
-                active_filters: 'Active Filters',
+                active_filters: 'Active filters',


I thought we were moving to sentence case for most things?

Is that not sentence case?

lol, don't mind me...

jamescdavis · 2018-10-05T13:41:10Z

lib/app-components/addon/helpers/math.ts

+    }
+
+    return -1;
+}


Are these functions useful outside this helper?

They're exported for testing purposes. I don't really see a need to test KaTeX itself.

Ah, ok, I was just wondering why they were exported (I obviously didn't look close enough at the tests!).

jamescdavis · 2018-10-05T13:48:52Z

lib/app-components/index.js

+        }
+
+        this.import(`${katexPath}/katex.css`);
+        this.import(`${katexPath}/katex.js`, { using: [{ transformation: 'amd', as: 'katex' }] });


Once we get ember-auto-import working for in-repo addons in TypeScript (which should work when typed-ember/ember-cli-typescript#334), this will be unnecessary, but, alas, it's the best we got atm.

jamescdavis · 2018-10-05T13:51:51Z

lib/osf-components/addon/components/osf-navbar/auth-dropdown/component.ts

+
+        if (this.router.currentRouteName !== 'register') {
+            params.next = this.signUpNext;
+        }


Not really sure, just copying the existing behavior.

return this.router.currentRouteName === 'register' ? {} : { next: this.signUpNext };

jamescdavis · 2018-10-05T13:55:02Z

lib/osf-components/addon/components/osf-navbar/template.hbs

                    {{/let}}

-                    {{osf-navbar/auth-dropdown}}
+                    {{osf-navbar/auth-dropdown campaign=this.campaign}}


campaign is a parameter that's just passed through right? Should use campaign=@campaign and remove the property from the component class.

jamescdavis · 2018-10-05T14:00:04Z

lib/registries/addon/components/registries-search-result/template.hbs

+            {{#each this.contributors as |contrib|}}
+                <li>
+                    {{#if contrib.link}}
+                        <a href={{contrib.link}}>{{contrib.name}}</a>


We'll eventually want this to be a link-to (or equivalent). I guess we'll parse out the route from the OSF URL in identifiers or something?

It'll probably look similar to the title? It'd be nice if we had a link-to that checked feature flags at somepoint

jamescdavis · 2018-10-05T14:03:20Z

lib/registries/addon/components/registries-search-result/template.hbs


        <div class="text-center">
-            <button data-test-result-toggle-id="{{result.id}}" local-class="RegistriesSearchResult__Toggle" class="btn btn-link" aria-label={{t 'eosf.components.searchResult.showResult'}} {{action 'toggleExpanded'}}>
+            <button {{action 'toggleExpanded'}} data-test-result-toggle-id={{result.id}} local-class="RegistriesSearchResult__Toggle" class="btn btn-link" aria-label={{t 'eosf.components.searchResult.showResult'}}>


I'm a fan of {{action this.toggleExpanded}}

jamescdavis · 2018-10-05T14:08:16Z

tests/engines/registries/unit/services/search-test.ts

 }

-module('Unit | Registries | Service | Search', hooks => {
+module('Unit | Service | search', hooks => {


Seems like we should leave Registries in the test name somewhere?
Maybe Registries | Unit | Service | search?

jamescdavis · 2018-10-05T16:03:42Z

app/locales/en/translations.ts

@@ -955,10 +955,10 @@ export default {
            sidebar: {
                refine_your_search: 'Refine your search by',
                clear_filters: 'Clear Filters',


sentence case this one too?

chrisseto force-pushed the feature/registries-fixes branch 3 times, most recently from b550bca to 3c47e75 Compare October 3, 2018 19:45

aaxelb reviewed Oct 4, 2018

View reviewed changes

chrisseto force-pushed the feature/registries-fixes branch from 3c47e75 to 3efcdcf Compare October 4, 2018 18:32

chrisseto force-pushed the feature/registries-fixes branch 4 times, most recently from e7ef604 to 89813dd Compare October 4, 2018 20:43

jamescdavis suggested changes Oct 5, 2018

View reviewed changes

jamescdavis previously approved these changes Oct 5, 2018

View reviewed changes

chrisseto added 5 commits October 5, 2018 12:01

Use shared search-help-modal and remove search-dropdown

b6c11ac

Render TeX expression in registries search results

86e2dd8

Properly unescape and link search results

e2b9d0a

Show all contributors in search results

513f04f

Reset to page 1 when changing queries

1832760

chrisseto dismissed jamescdavis’s stale review via a0ee70e October 5, 2018 16:02

chrisseto force-pushed the feature/registries-fixes branch from 817312e to a0ee70e Compare October 5, 2018 16:02

jamescdavis reviewed Oct 5, 2018

View reviewed changes

chrisseto added 4 commits October 5, 2018 12:05

Use sentence casing for consistency

8814645

Use shared paginator

9035583

Allow customizing the signup campaign

53d73e9

Disable no-restricted-globals

71aa756

chrisseto force-pushed the feature/registries-fixes branch from a0ee70e to 71aa756 Compare October 5, 2018 16:05

jamescdavis approved these changes Oct 5, 2018

View reviewed changes

chrisseto merged commit 7203f22 into CenterForOpenScience:develop Oct 5, 2018

jamescdavis added this to the 18.0.0 milestone Oct 5, 2018

		@@ -0,0 +1,40 @@
		import { layout } from '@ember-decorators/component';

Conversation

chrisseto commented Oct 3, 2018

Purpose

Summary of Changes

Side Effects

Feature Flags

QA Notes

Ticket

Reviewer Checklist

Uh oh!

aaxelb left a comment

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!

coveralls commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1663

💛 - Coveralls

Uh oh!

jamescdavis left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Oct 4, 2018 •

edited

Loading