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

Capitalize help text for when expressions in the task sidebar #9235

Merged

Conversation

debsmita1
Copy link
Contributor

@debsmita1 debsmita1 commented Jun 14, 2021

@openshift-ci openshift-ci bot added component/pipelines Related to pipelines-plugin kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Jun 14, 2021
@debsmita1
Copy link
Contributor Author

/cc @bgliwa01
/assign @andrewballantyne

@andrewballantyne
Copy link
Contributor

This covers the situation... I want to now go on an easter egg hunt to see if anything else is missing. So Brigid doesn't need to log another issue in a few days 😄

@bgliwa01
Copy link

Help text looks good! The dropdown for operator should be "Select operator" in order to comply with sentence case

@andrewballantyne
Copy link
Contributor

andrewballantyne commented Jun 14, 2021

For posterity, I've inquired with @bgliwa01 about the two different wordings we have for using $(...

Ah, interesting, we have two wordings
Use this format when referencing variables in this form: <code>$(</code> (params)
vs
Use this format while referencing the variables in this form: <code>$(</code> (when)

@andrewballantyne
Copy link
Contributor

@debsmita1 Would you mind updating some of the error text to have proper sentence case; missing period in messages found in frontend/packages/pipelines-plugin/src/components/pipelines/pipeline-builder/validation-utils.ts:

  • Resource type has changed, reselect
  • Workspace name has changed, reselect
  • TaskSpec or TaskRef must be provided
  • Must define at least one Task

Last two may not appear today, but it's best to just align it.

@bgliwa01
Copy link

For posterity, I've inquired with @bgliwa01 about the two different wordings we have for using $(...

Ah, interesting, we have two wordings
Use this format when referencing variables in this form: <code>$(</code> (params)
vs
Use this format while referencing the variables in this form: <code>$(</code> (when)

After talking to content, it should be "Use this format when you reference variables in this form: $("

@debsmita1
Copy link
Contributor Author

@andrewballantyne Should I change Must define at least one Task to Must define at least one task ? or should I simply add period to this ?

@andrewballantyne
Copy link
Contributor

@andrewballantyne Should I change Must define at least one Task to Must define at least one task ? or should I simply add period to this ?

Oooh, yeah task please. Task is not the resource, it's a concept in the Pipeline. Good catch.

@debsmita1
Copy link
Contributor Author

@andrewballantyne Should I change Must define at least one Task to Must define at least one task ? or should I simply add period to this ?

Oooh, yeah task please. Task is not the resource, it's a concept in the Pipeline. Good catch.

Aslo ,
Screenshot from 2021-06-15 00-05-00
and
Screenshot from 2021-06-15 00-06-31

to Remove task and Select task respectively ?

@andrewballantyne
Copy link
Contributor

@andrewballantyne Should I change Must define at least one Task to Must define at least one task ? or should I simply add period to this ?

Oooh, yeah task please. Task is not the resource, it's a concept in the Pipeline. Good catch.

Aslo ,
Screenshot from 2021-06-15 00-05-00
and
Screenshot from 2021-06-15 00-06-31

to Remove task and Select task respectively ?

Ah yeah, same reason. I read that and went "yeah, no that's correct" but in reality it is not. Also please change the modal.

@debsmita1
Copy link
Contributor Author

/kind bug

@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 14, 2021
@@ -98,7 +98,7 @@ const PipelineSecretSection: React.FC = () => {
className="odc-pipeline-secret-section__secret-action"
icon={<PlusCircleIcon />}
>
{t('pipelines-plugin~Add Secret')}
{t('pipelines-plugin~Add secret')}
Copy link
Contributor

Choose a reason for hiding this comment

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

Secret is a k8s resource. Capital is correct I believe.

cc @bgliwa01

Copy link
Member

Choose a reason for hiding this comment

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

Just fyi:

image

But I also know that Secret is here "exact a Secret resource" and the values in the dropdown are talking about the concept "secret" ... Anyway a little bit confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

A fair point 🤷

Choose a reason for hiding this comment

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

When secret is referred to as a resource it should be capital

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@andrewballantyne
Copy link
Contributor

There are some test failures too, that vet text you modified.

@debsmita1
Copy link
Contributor Author

There are some test failures too, that vet text you modified.

Fixed the failing tests

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2021
Copy link

@bgliwa01 bgliwa01 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@jerolimov jerolimov left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2021
@andrewballantyne
Copy link
Contributor

/hold

@debsmita1 can we address the Secret issue I mentioned above before merge to avoid another bug to address it.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2021
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 15, 2021
@debsmita1
Copy link
Contributor Author

/hold

@debsmita1 can we address the Secret issue I mentioned above before merge to avoid another bug to address it.

Done

Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

/lgtm
/unhold

Thanks @debsmita1 !

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 15, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 15, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, bgliwa01, debsmita1, jerolimov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit fe0f165 into openshift:master Jun 16, 2021
@spadgett spadgett added this to the v4.9 milestone Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/pipelines Related to pipelines-plugin kind/bug Categorizes issue or PR as related to a bug. kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants