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

Restyle file uploads to fit in with modern forms UI #7452

Merged
merged 11 commits into from
Dec 30, 2022

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Nov 26, 2022

A small PR to update the file upload control so that it fits in with the rest of forms. The button now resembles a standard .jenkins-button rather than the native OS one.

Before

image

After

image

Testing done

Go to a page with a file upload control, such as Plugin Manager -> Advanced.
Spot the file upload control.
Upload a file.
File uploads successfully.

Proposed changelog entries

  • Restyle file uploads to fit in with modern forms UI

Proposed upgrade guidelines

N/A

Submitter checklist

  • The Jira issue, if it exists, is well-described.
  • The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
    • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
  • There is automated testing or an explanation as to why this change has no tests.
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
  • New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
  • For dependency updates, there are links to external changelogs and, if possible, full differentials.
  • For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@jenkinsci/sig-ux

Maintainer checklist

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

  • There are at least two (2) approvals for the pull request and no outstanding requests for change.
  • Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
  • Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
  • Proper changelog labels are set so that the changelog can be generated automatically.
  • If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@NotMyFault NotMyFault 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 Nov 26, 2022
Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Screenshot 2022-11-26 at 10 23 28
Screenshot 2022-11-26 at 10 23 34

Is it intended to truncate the file name? Looks a bit odd to me. So is the missing space between the button and the file to upload.

Maybe we should add a page to the design lib outlining that you should use the jenkins-file-upload class when accepting file inputs, rather than jenkins-upload or setting-upload 🤔

@janfaracik
Copy link
Contributor Author

janfaracik commented Nov 26, 2022

Screenshot 2022-11-26 at 10 23 28

Is it intended to truncate the file name? Looks a bit odd to me

image

Good spot, hadn't tried it with a really long file name - just needed to set the width to 100%.

So is the missing space between the button and the file to upload.

Yeah it's a limitation with the upload input :/ I've tried everything I can think of but still no additional padding. It's a limitation in Safari, just tried it in Chrome and the margin property works as expected 🎉

Screenshot 2022-11-26 at 21 36 21

Maybe we should add a page to the design lib outlining that you should use the jenkins-file-upload class when accepting file inputs, rather than jenkins-upload or setting-upload 🤔

Definitely 👍

@NotMyFault NotMyFault requested a review from a team December 1, 2022 11:31
@daniel-beck
Copy link
Member

Would it not be easier / more backwards compatible to apply different rules for input type="file" rather than requiring every declaration to be adapted to what amounts to redundant information? Or would there be input type="file" that should not have class="jenkins-file-upload"?

@uhafner
Copy link
Member

uhafner commented Dec 1, 2022

Would it not be easier / more backwards compatible to apply different rules for input type="file" rather than requiring every declaration to be adapted to what amounts to redundant information? Or would there be input type="file" that should not have class="jenkins-file-upload"?

Actually this calls for a jelly template f:fileButton as well (encapsulation).

@daniel-beck
Copy link
Member

Actually this calls for a jelly template f:fileButton as well (encapsulation).

Not opposed (I've previously impersonated a broken record asking for Jelly components instead of changing CSS classes around), but only doing that still wouldn't take care of existing upload fields.

@timja
Copy link
Member

timja commented Dec 29, 2022

Would it not be easier / more backwards compatible to apply different rules for input type="file" rather than requiring every declaration to be adapted to what amounts to redundant information? Or would there be input type="file" that should not have class="jenkins-file-upload"?

see https://weekly.ci.jenkins.io/design-library/Conventions/. All new CSS is to be name-spaced and prefixed to prevent clashed with other libraries etc.

If we were to modify the default styling then this would be the only component I'm aware of doing it that way and would be inconsistent with everything else.

@timja
Copy link
Member

timja commented Dec 29, 2022

I've filed PRs with all plugins that look to be maintained using this search: https://github.com/search?q=org%3Ajenkinsci+input+type%3D%22file%22+NOT+repo%3Ajenkinsci%2Fjenkins&type=code&p=1

Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Looks good, left some optional feedback for consideration.

core/src/main/resources/lib/form/file.jelly Show resolved Hide resolved
Co-authored-by: Alexander Brandes <brandes.alexander@web.de>
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 29, 2022
@timja timja merged commit ce4f9de into jenkinsci:master Dec 30, 2022
@timja timja deleted the style-file-upload branch December 30, 2022 20:23
@jglick
Copy link
Member

jglick commented Jan 3, 2023

I've filed PRs with all plugins that look to be maintained using this search

Looks like you missed file-parameters-plugin, because you did not search for an intervening attribute (name in that case).

Enable structured form submission.
</st:attribute>
<st:attribute name="accept">
Defines the file types the file input should accept. This string is a comma-separated list.
Copy link
Member

Choose a reason for hiding this comment

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

Optional I guess?

You could just copy unknown tag attrs to the HTML tag. There is some Jelly/Stapler trick to do that.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, use="required" denotes mandatory.

if can you point to it that could be useful although likely unneeded

Copy link
Contributor

Choose a reason for hiding this comment

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

You could just copy unknown tag attrs to the HTML tag. There is some Jelly/Stapler trick to do that.

Second this as it is important for my use case.

Copy link
Member

Choose a reason for hiding this comment

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

* Tags from this namespace ("jelly:hudson.util.jelly.MorphTagLibrary") behaves mostly like literal static tags,
* except it interprets two attributes "ATTRIBUTES" and "EXCEPT" in a special way.
*
* The "ATTRIBUTES" attribute should have a Jelly expression that points to a {@link Map} object,
* and the contents of the map are added as attributes of this tag, with the exceptions of entries whose key
* values are listed in the "EXCEPT" attribute.
*
* The "EXCEPT" attribute takes a white-space separated list of attribute names that should be ignored even
* if it's in the map.
<m:input xmlns:m="jelly:hudson.util.jelly.MorphTagLibrary"
class="jenkins-input ${attrs.checkUrl!=null?'validated':''} ${attrs.autoCompleteUrl!=null?'auto-complete':null} ${attrs.clazz}"
name="${name}"
value="${value}"
type="text"
placeholder="${placeholder}"
ATTRIBUTES="${attrs}" EXCEPT="field clazz" />

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, @strangelookingnerd what is missing that you need?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

That source file should be st:adjunct and probably Behaviour.specify but that is another story.

Copy link
Member

Choose a reason for hiding this comment

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

Styling should be done via classes and not inline css, I added support for an ID because of your plugin but you could also do that via a CSS class anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback 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
7 participants