Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENG-3884] Added the component file, navbar and links for the ticket #2489

Merged

Conversation

bp-cos
Copy link
Contributor

@bp-cos bp-cos commented Jan 22, 2025

Caveat - some of these files are very old. Since the last time the files were saved, new formatting rules have been added which have caused more changes than necessary for the completion of the project.

Purpose

Add a components navbar link and page to the legacy projects section.

Summary of Changes

Add a component
Add a route
Add new translations

Screenshot(s)

Screenshot 2025-01-22 at 11 55 02 AM

Screenshot 2025-01-22 at 11 56 49 AM

Side Effects

Ticket ENG-6840 needs to be completed before this ticket can be tested and deployed.

https://openscience.atlassian.net/browse/ENG-6840

QA Notes

Ticket ENG-6840 needs to be completed before this ticket can be tested and deployed.

Copy link
Contributor

@futa-ikeda futa-ikeda left a comment

Choose a reason for hiding this comment

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

Just a couple minor things. A couple of basic acceptance tests (e.g. no components, one or more components) for this route would nice as well.

.vscode/settings.json Outdated Show resolved Hide resolved
lib/osf-components/addon/components/node-card/component.ts Outdated Show resolved Hide resolved
Comment on lines 204 to 373
{{#if @node.isProject}}
<div>
<dt data-test-resource-type-label>
{{t 'node_card.resource-type'}}
</dt>
<dd data-test-resource-type>
{{this.resourceType}}
</dd>
</div>
{{/if}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this.resourceType is null while this.getGuidMetadata is running, this is going to show Resource Type: <blank>. Do we want to add some sort of loading state? Something like

Suggested change
{{#if @node.isProject}}
<div>
<dt data-test-resource-type-label>
{{t 'node_card.resource-type'}}
</dt>
<dd data-test-resource-type>
{{this.resourceType}}
</dd>
</div>
{{/if}}
{{#if @node.isProject}}
{{#if this.getGuidMetadata.isRunning}}
<LoadingIndicator @dark={{true}} @inline={{true}} />
{{else}}
<div>
<dt data-test-resource-type-label>
{{t 'node_card.resource-type'}}
</dt>
<dd data-test-resource-type>
{{this.resourceType}}
</dd>
</div>
{{/if}}
{{/if}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is going to be replaced by the CpPanel and will fix the "empty" situation.

Copy link
Contributor

@futa-ikeda futa-ikeda left a comment

Choose a reason for hiding this comment

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

One minor thing and one more thing re: resourceType section that I'll message you on slack about

Comment on lines 114 to 119
async toggleResourceType() {
this.isOpenResourceType = !this.isOpenResourceType;
if (this.isOpenResourceType) {
taskFor(this.getGuidMetadata).perform();
}
}
Copy link
Contributor

@futa-ikeda futa-ikeda Jan 23, 2025

Choose a reason for hiding this comment

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

A couple of minor things here:

  1. No need for the async here, as this isn't awaiting anything
  2. The condition on line 116 will trigger the task to run every time the panel is opened, so could we do something like
if (this.isOpenResourceType && !this.resourceType) {
    taskFor(this.getGuidMetadata).perform();
}

to ensure that this task only runs when needed?

Edit: Scratch point 2. Seems that this action should only really be called once

Comment on lines 106 to 107
const metadataRecord =
(await guidRecord.customMetadata) as CustomFileMetadataRecordModel;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This should be Custom Item MetadataRecordModel, not Custom File MetadataRecordModel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Caught me copying and pasting!

Comment on lines 309 to 341
<CpPanel
id={{@node.id}}
@open={{this.isOpenResourceType}}
as |panel|
>
<panel.body>
{{#if this.resourceType}}
{{this.resourceType}}
{{else}}
<LoadingIndicator
@dark={{true}}
/>
{{/if}}
</panel.body>
</CpPanel>
{{#unless this.isOpenResourceType}}
<Button
{{on
'click'
this.toggleResourceType
}}
aria-label='{{t 'node_card.resource-type.show-resource-type' }}'
aria-controls={{@node.id}}
aria-expanded={{this.isOpenResourceType}}
>
<FaIcon @icon={{'angle-down'}} />
<EmberTooltip>
{{t
'node_card.resource-type.show-resource-type'
}}
</EmberTooltip>
</Button>
{{/unless}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this was the implementation that was in mind when talking about the "disclosure triangle". As per my earlier slack message, check the search page's result cards for this type of feature. The CpPanel and the dropdown in this implementation seems unneeded/inappropriate. Could just be something simple like

Suggested change
<CpPanel
id={{@node.id}}
@open={{this.isOpenResourceType}}
as |panel|
>
<panel.body>
{{#if this.resourceType}}
{{this.resourceType}}
{{else}}
<LoadingIndicator
@dark={{true}}
/>
{{/if}}
</panel.body>
</CpPanel>
{{#unless this.isOpenResourceType}}
<Button
{{on
'click'
this.toggleResourceType
}}
aria-label='{{t 'node_card.resource-type.show-resource-type' }}'
aria-controls={{@node.id}}
aria-expanded={{this.isOpenResourceType}}
>
<FaIcon @icon={{'angle-down'}} />
<EmberTooltip>
{{t
'node_card.resource-type.show-resource-type'
}}
</EmberTooltip>
</Button>
{{/unless}}
{{#if this.resourceType}}
{{this.resourceType}}
{{else}}
<LoadingIndicator
@dark={{true}}
/>
{{/if}}
{{#unless this.isOpenResourceType}}
<Button
{{on
'click'
this.toggleResourceType
}}
>
{{t
'node_card.resource-type.show-resource-type'
}}
</Button>
{{/unless}}

Copy link
Contributor

@futa-ikeda futa-ikeda left a comment

Choose a reason for hiding this comment

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

Not a fan of the design, but if that's what they want...

@bp-cos bp-cos merged commit 30fe315 into CenterForOpenScience:feature/b-and-i-25-01 Jan 30, 2025
9 checks passed
@bp-cos bp-cos deleted the feature/eng-3884 branch January 30, 2025 21:08
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.

3 participants