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

feat(KFLUXUI-36): updated wordings in customizepipelines component fo… #96

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

rakshett
Copy link
Contributor

@rakshett rakshett commented Jan 27, 2025

Fixes

https://issues.redhat.com/browse/KFLUXUI-36

Description

  1. Updated wordings in CustomizePipelines component for install gitHub application.
  2. Removed Kebab action dropdown from the rows.
  3. Removed Rollback to default pipeline button from the alert.

Type of change

  • Feature
  • Bugfix
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Screen shots / Gifs for design review

Screenshot 2025-02-04 at 3 15 33 PM

How to test or reproduce?

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

Copy link

codecov bot commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.08%. Comparing base (93f0fc3) to head (3d99df2).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #96      +/-   ##
==========================================
+ Coverage   80.06%   80.08%   +0.02%     
==========================================
  Files         544      544              
  Lines       21155    21138      -17     
  Branches     5326     5059     -267     
==========================================
- Hits        16937    16928       -9     
+ Misses       4194     4186       -8     
  Partials       24       24              
Flag Coverage Δ
unittests 80.08% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
.../CustomizedPipeline/CustomizeComponentPipeline.tsx 91.11% <ø> (ø)
...mponents/CustomizedPipeline/CustomizePipelines.tsx 92.50% <100.00%> (+4.90%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93f0fc3...3d99df2. Read the comment docs.

@sahil143
Copy link
Collaborator

/test

Copy link
Collaborator

@sahil143 sahil143 left a comment

Choose a reason for hiding this comment

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

Screenshot 2025-01-27 at 1 46 19 PM
  • Remove Kebab action dropdown from the rows
  • Learn more links look weird, gitlab link should be in next line.
  • Remove Rollback to default pipeline button from the alert
  • Why is text not center aligned like before?

src/components/CustomizedPipeline/CustomizePipelines.tsx Outdated Show resolved Hide resolved
Comment on lines 344 to 345
const gitlabURL =
'https://konflux-ci.dev/docs/how-tos/configuring/creating-secrets/#creating-source-control-secrets';
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use the link in component, no need to create a variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also in some ticket we changed gitlab and github naming to git.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the changes.

@rakshett rakshett requested a review from sahil143 February 4, 2025 09:57
@sahil143
Copy link
Collaborator

/test

@@ -387,34 +296,43 @@ const CustomizePipeline: React.FC<React.PropsWithChildren<Props>> = ({
<>
<TextContent
className="pf-v5-u-text-align-center pf-v5-u-pt-lg"
style={{ visibility: allLoading ? 'hidden' : undefined }}
style={{ visibility: allLoading ? 'hidden' : undefined, textAlign: 'center' }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

pf-v5-u-text-align-center should align the text to the center?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well this classname is not available now as part of v5 now

</Text>
<Text component={TextVariants.p}>
<ExternalLink
href={githubAppURL}
style={{ paddingLeft: 'var(--pf-v5-global--spacer--2xl)' }}
href={'https://pipelinesascode.com/docs/install/github_apps/#manual-setup'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be a link to install GitHub app?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Link is added as per https://issues.redhat.com/browse/KFLUXUI-36. Please let me know if any change required

repository. With custom build pipelines, commits to your main branch and pull requests
will automatically rebuild. You can always roll back to default.
</Text>
<Text component={TextVariants.h2}>{'Manage build pipeline'}</Text>
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
<Text component={TextVariants.h2}>{'Manage build pipeline'}</Text>
<Text component={TextVariants.h2}>Manage build {pluralize(components.length, 'pipeline')}</Text>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

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