Skip to content

Conversation

@fdiblen
Copy link
Member

@fdiblen fdiblen commented Aug 22, 2022

Pull request details

Adds a dialog to the start screen which shows description, examples, and documentation link for a field.

List of related issues or pull requests

Refs:

Describe the changes made in this pull request

Instructions to review the pull request

@github-actions
Copy link

Once the build has completed, you can preview your PR at this URL: https://cffinit.netlify.app/PR646/

@github-actions
Copy link

Once the build has completed, you can preview your PR at this URL: https://cffinit.netlify.app/PR646/

@github-actions
Copy link

Once the build has completed, you can preview your PR at this URL: https://cffinit.netlify.app/PR646/

@github-actions
Copy link

Once the build has completed, you can preview your PR at this URL: https://cffinit.netlify.app/PR646/

@github-actions
Copy link

Once the build has completed, you can preview your PR at this URL: https://cffinit.netlify.app/PR646/

@github-actions
Copy link

Once the build has completed, you can preview your PR at this URL: https://cffinit.netlify.app/PR646/

Copy link
Collaborator

@abelsiqueira abelsiqueira left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I made some suggestions which I think are relevant for future PRs and will be harder to change later. They are:

  • I think examples should be optional
  • data.title should be uppercase (Type, Related Resources), etc.

Either way, I think this is good to go.

}
const helpData = {
type: {
title: 'type',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
title: 'type',
title: 'Type',

Copy link
Collaborator

Choose a reason for hiding this comment

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

repository-artifact field


Artifact Repository

CFF Field: repository-artifact

]
},
message: {
title: 'message',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
title: 'message',
title: 'Message',

]
},
title: {
title: 'title',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
title: 'title',
title: 'Title',

src/css/app.css Outdated
Comment on lines 119 to 122
#help-dialog {
background-color: var(--fgcolor, lightslategray);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would put these scoped in the component.

<q-card-section>
{{ data.description }}
</q-card-section>
<q-card-section>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<q-card-section>
<q-card-section v-if="data.examples">

I would make this optional since sometimes there are no examples (radio for types, for instance).

Comment on lines 124 to 128
description: 'The type of the work that is being described by this CITATION.cff file.',
examples: [
'software',
'dataset'
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: 'The type of the work that is being described by this CITATION.cff file.',
examples: [
'software',
'dataset'
]
description: 'The type of the work that is being described by this CITATION.cff file.'

If we make examples optional.

},
data: {
type: Object,
required: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
required: false,
required: true,

}
const helpData = {
type: {
title: 'type',
Copy link
Collaborator

Choose a reason for hiding this comment

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

repository-artifact field


Artifact Repository

CFF Field: repository-artifact

Copy link
Collaborator

@abelsiqueira abelsiqueira left a comment

Choose a reason for hiding this comment

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

I'm gonna have another look later

@github-actions
Copy link

Once the build has completed, you can preview your PR at this URL: https://cffinit.netlify.app/PR646/

@github-actions
Copy link

Once the build has completed, you can preview your PR at this URL: https://cffinit.netlify.app/PR646/

@fdiblen fdiblen marked this pull request as ready for review August 24, 2022 10:19
@fdiblen fdiblen requested a review from abelsiqueira August 24, 2022 10:20
Copy link
Collaborator

@abelsiqueira abelsiqueira left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the updates.

@fdiblen fdiblen merged commit 737162c into main Aug 24, 2022
@fdiblen fdiblen deleted the 635-info-dialog branch August 24, 2022 11:14
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