Skip to content

Show full CSV description as markdown in dev catalog item details #3734

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 1 commit into from
Jan 3, 2020
Merged

Show full CSV description as markdown in dev catalog item details #3734

openshift-merge-robot merged 1 commit into from
Jan 3, 2020

Conversation

@rohitkrai03
Copy link
Contributor

@rohitkrai03 rohitkrai03 commented Dec 9, 2019 •
edited
Loading

Related Story - https://jira.coreos.com/browse/ODC-2457

This PR -

  • Updates the catalog item details page to show full CSV description as markdown.
  • It includes both the CRD description and CSV description.

Screenshots -

Before -
Screenshot from 2019-12-10 02-39-17

After -
Screenshot from 2019-12-10 02-37-59

@openshift-ci-robot openshift-ci-robot added component/core Related to console core functionality component/olm Related to OLM size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 9, 2019
@christianvogt
Copy link
Contributor

christianvogt commented Dec 9, 2019

fyi @sspeiche @openshift/team-devconsole-ux

@serenamarie125
Copy link
Contributor

serenamarie125 commented Dec 9, 2019

@rohitkrai03 can we possibly change the format?

Description
CRD description

Operator Description
CSV description

@sspeiche
Copy link

sspeiche commented Dec 10, 2019

fyi @dmesser

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 10, 2019
@rohitkrai03
Copy link
Contributor Author

rohitkrai03 commented Dec 10, 2019

@rohitkrai03 can we possibly change the format?

Description
CRD description

Operator Description
CSV description

@serenamarie125 Updated as per your suggestion. This is how it looks like now -

Screenshot from 2019-12-11 01-39-40

@serenamarie125
Copy link
Contributor

serenamarie125 commented Dec 11, 2019

@rohitkrai03 I think there's a hierarchy issue because the operator name is larger than the description titles. Can you please make Description & Operator Description h1s?

@christianvogt
Copy link
Contributor

christianvogt commented Dec 11, 2019

If we change those to h1, then what about the operator details page:
image

In this example, the Provided APIs and the Description are also currently smaller.

@serenamarie125
Copy link
Contributor

serenamarie125 commented Dec 11, 2019

@itsptk do you think you'd make the same change there ?

@itsptk
Copy link

itsptk commented Dec 11, 2019

My initial take is that this particular Knative Serving operator just happens to use h1 styling for that "Knative Serving" text but several other operators I checked mostly used h2 or smaller. My inclination would be to not update our existing styling to accommodate this particular operator's description hierarchy.

@christianvogt
Copy link
Contributor

christianvogt commented Dec 11, 2019

The other thing we could do is to stylizes our markdown in this area so that h1's are the same as h2's ?

@itsptk
Copy link

itsptk commented Dec 11, 2019

The other thing we could do is to stylizes our markdown in this area so that h1's are the same as h2's ?

I spoke with @maryshak1996 about this idea and I think we would go this direction in the admin console instead of making the Description headers h1s, as that would better fit the hierarchy in the installed operator details.

@rohitkrai03
Copy link
Contributor Author

rohitkrai03 commented Dec 16, 2019

/test e2e-gcp-console

invincibleJai
invincibleJai reviewed Dec 16, 2019
View reviewed changes
frontend/packages/operator-lifecycle-manager/src/dev-catalog.ts Outdated
Copy link
Member

@invincibleJai invincibleJai Dec 16, 2019

Choose a reason for hiding this comment

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

nit: type for desc would be good to have

Copy link
Contributor Author

@rohitkrai03 rohitkrai03 Dec 17, 2019

Choose a reason for hiding this comment

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

Actually description is being reduced using multiple resources so its hard to define a strict type for it and out of scope for this story. But I have refactored the function to get more specific params with strict types.

invincibleJai
invincibleJai reviewed Dec 16, 2019
View reviewed changes
frontend/packages/operator-lifecycle-manager/src/dev-catalog.ts Outdated
Copy link
Member

@invincibleJai invincibleJai Dec 16, 2019

Choose a reason for hiding this comment

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

is there a possibility when desc.csv.spec.description would get undefined

Copy link
Contributor Author

@rohitkrai03 rohitkrai03 Dec 17, 2019

Choose a reason for hiding this comment

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

Don't think so. The CSV data should contain a description.

Copy link
Contributor

@andrewballantyne andrewballantyne Dec 17, 2019

Choose a reason for hiding this comment

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

Just an FYI, we did upgrade TypeScript to 3.7 and there is something called Optional Chaining now https://devblogs.microsoft.com/typescript/announcing-typescript-3-7/#optional-chaining - so checking for missing items is less of a concern.

@invincibleJai
Copy link
Member

invincibleJai commented Dec 17, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2019
@invincibleJai
Copy link
Member

invincibleJai commented Dec 17, 2019

/lgtm cancel

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2019
@invincibleJai
Copy link
Member

invincibleJai commented Dec 17, 2019

/lgtm

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 27, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

20 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 27, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 28, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@rohitkrai03
Copy link
Contributor Author

rohitkrai03 commented Dec 28, 2019

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 28, 2019
@andrewballantyne
Copy link
Contributor

andrewballantyne commented Jan 2, 2020

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 2, 2020
@andrewballantyne
Copy link
Contributor

andrewballantyne commented Jan 2, 2020

/hold cancel
/retest
#3847 has fixed the e2e tests

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 2, 2020
@openshift-merge-robot openshift-merge-robot merged commit 14397e9 into openshift:master Jan 3, 2020
@spadgett spadgett added this to the v4.4 milestone Jan 3, 2020
@rohitkrai03 rohitkrai03 deleted the full-md-catalog branch March 30, 2020 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeff-phillips-18 jeff-phillips-18 Awaiting requested review from jeff-phillips-18

@spadgett spadgett Awaiting requested review from spadgett

2 more reviewers

@invincibleJai invincibleJai invincibleJai left review comments

@andrewballantyne andrewballantyne andrewballantyne left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@invincibleJai invincibleJai

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/olm Related to OLM kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

v4.4

Development

Successfully merging this pull request may close these issues.