-
Notifications
You must be signed in to change notification settings - Fork 72
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
Dialogs #264
Dialogs #264
Conversation
src/main/resources/io/jenkins/plugins/designlibrary/Dialogs/index.jelly
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow really nice, minor feedback and typos:
src/main/resources/io/jenkins/plugins/designlibrary/Dialogs/index.properties
Outdated
Show resolved
Hide resolved
src/main/resources/io/jenkins/plugins/designlibrary/Dialogs/index.properties
Outdated
Show resolved
Hide resolved
src/main/resources/io/jenkins/plugins/designlibrary/Dialogs/index.jelly
Outdated
Show resolved
Hide resolved
src/main/resources/io/jenkins/plugins/designlibrary/Dialogs/index.properties
Outdated
Show resolved
Hide resolved
src/main/resources/io/jenkins/plugins/designlibrary/Dialogs/index.properties
Outdated
Show resolved
Hide resolved
Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>
All dialogs take a second optional parameter with options that allow to change certain aspects. | ||
<ul> | ||
<li><code>title</code>: Adds a title to the dialog</li> | ||
<li><code>okText</code>: Adjust the text of the OK button, defaults to <code>OK</code> or <code>Yes</code></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A default should be a single value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default depends on the context. It is as of now Yes
for confirm, Submit
for forms and OK
otherwise. Maybe that info should be moved to the corresponding dialog type page.
<st:adjunct includes="io.jenkins.plugins.designlibrary.Dialogs.dialogs"/> | ||
<script src="${resURL}/jsbundles/section-to-tabs.js" type="text/javascript" /> | ||
</s:sample> | ||
</j:jelly> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires a SNAPSHOT version of Jenkins to see that in action? For me the execute button simply throws an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then you're not on the latest head of this PR. It uses an incremental version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deployed the plugin into my latest Jenkins instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you run this with mvn hpi:run
then you will get the incremental version automatically
or you can open in gitpod:
https://gitpod.io/#https://github.com/jenkinsci/design-library-plugin/pull/264
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
switch title and message
src/main/resources/io/jenkins/plugins/designlibrary/Dialogs/dialogs.css
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great, two nits I noticed
src/main/resources/io/jenkins/plugins/designlibrary/Dialogs/index.jelly
Outdated
Show resolved
Hide resolved
Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>
…dex.jelly Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>
@@ -44,7 +44,8 @@ THE SOFTWARE. | |||
<properties> | |||
<changelist>999999-SNAPSHOT</changelist> | |||
<gitHubRepo>jenkinsci/design-library-plugin</gitHubRepo> | |||
<jenkins.version>2.385</jenkins.version> | |||
<!-- TODO https://github.com/jenkinsci/jenkins/pull/7938 --> | |||
<jenkins.version>2.414-rc33991.44dfeeff7872</jenkins.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an incremental it shouldn’t have been merged
See jenkinsci/jenkins#7938
Testing done
Submitter checklist