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

Yui button inspired style #10061

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

Conversation

scherler
Copy link
Contributor

@scherler scherler commented Dec 16, 2024

Testing done

image

  • .jenkins-button--outline
    image
  • .jenkins-button--outline (on hover)

Proposed changelog entries

  • human-readable text

Proposed upgrade guidelines

N/A

Submitter checklist

Preview Give feedback

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Preview Give feedback

@janfaracik
Copy link
Contributor

Heyo. The existing classes for Button should work for your primary/destructive use cases - see https://weekly.ci.jenkins.io/design-library/Buttons/

image

e.g. jenkins-button jenkins-button--primary jenkins-!-destructive-color and jenkins-button jenkins-button--primary jenkins-!-success-color

You'd want to add an 'outline' modifier though for the 'Analyse now' button, e.g. jenkins-button jenkins-button--outline unless the default button styling could work instead?

@scherler
Copy link
Contributor Author

@janfaracik makes a lot of sense and thanks for pointing it out (I will follow up with a sample in the design-lib) I renamed the outline example

@scherler
Copy link
Contributor Author

jenkinsci/design-library-plugin#360

@timja timja added web-ui The PR includes WebUI changes which may need special expertise rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Dec 17, 2024
@timja timja requested a review from janfaracik December 17, 2024 14:05
@@ -75,6 +75,35 @@
}
}

.jenkins-button--outline {
color: var(--btn-secondary-color) !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

--btn-* vars are deprecated (YUI only), would just use var(--text-color) if it needs to be set.

.jenkins-button--outline {
color: var(--btn-secondary-color) !important;
border: 2px solid var(--btn-secondary-border);
font-weight: bold;
Copy link
Contributor

Choose a reason for hiding this comment

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

These props could be inherited from the base jenkins-button class.

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 but the original yui outline button had been bold, but if you want we can change it but IMHO it makes more sense bold.
image
vs
image


&:not(:disabled) {
&:hover {
border: 2px solid var(--accent-color) !important;
Copy link
Contributor

@janfaracik janfaracik Dec 18, 2024

Choose a reason for hiding this comment

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

This wouldn't work if paired with colour modifiers. We've been using currentColor in our other button modifiers to inherit the colour from modifiers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm are you sure
image
however I added jenkins-button--outline[class*="color"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jenkins-button jenkins-button--outline jenkins-!-success-color
image
:hover
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Heya, I'm travelling today but I'll take a look as soon as I can :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good traveling and MCAAHNY

@scherler scherler requested a review from janfaracik December 20, 2024 11:23
Copy link

@A1exKH A1exKH left a comment

Choose a reason for hiding this comment

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

@scherler LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants