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

MYFACES-4390: HtmlInputFile multiple support #191

Merged
merged 1 commit into from
Mar 31, 2021
Merged

MYFACES-4390: HtmlInputFile multiple support #191

merged 1 commit into from
Mar 31, 2021

Conversation

melloware
Copy link
Contributor

I feel like an alien in this code base!

I didn't see any existing tests to update so i wasn't even sure where to begin to add a test for this?

@melloware
Copy link
Contributor Author

melloware commented Mar 30, 2021

Also there were some warnings from IntelliJ about some evaluations ALWAYS being true etc but I did not fix any of them only touched the bare minimum to add this feature.

image

@melloware
Copy link
Contributor Author

melloware commented Mar 30, 2021

Also unsure as its mentioned it should render multiple="multiple" but right now I think it would render multiple="true". I still think multiple="true" is OK.

@tandraschko
Copy link
Member

why not
writer.writeAttribute(HTML.MULTIPLE_ATTR, "multiple", null); then?

@melloware
Copy link
Contributor Author

OK I updated it. to write out "multiple" but what threw me off is the lines above it were this...

if (inputFile.isDisabled())
{
     writer.writeAttribute(HTML.DISABLED_ATTR, Boolean.TRUE, null);
}

But in PrimeFaces code we do this...

 writer.writeAttribute("disabled", "disabled", null);

@GedMarc
Copy link

GedMarc commented Mar 31, 2021

yeah disabled=true doesn't work across all browsers, has to be just "disabled" or "disabled=disabled",

Multiple is the same, cant be multiple=true, must be multiple=multiple, or just "multiple"

@GedMarc
Copy link

GedMarc commented Mar 31, 2021

ref here https://stackoverflow.com/questions/6961526/what-is-the-correct-value-for-the-disabled-attribute

Don't use attributes=true for these at all, ever

@melloware
Copy link
Contributor Author

Thanks @GedMarc. So @tandraschko so then the Disabled attribute here is incorrect then.

@tandraschko
Copy link
Member

@melloware would you please add a jira issue and provide a PR? would be great!

@tandraschko tandraschko merged commit 9eff0f5 into apache:master Mar 31, 2021
@melloware
Copy link
Contributor Author

Yep will do!

@melloware melloware deleted the MYFACES-4390 branch March 31, 2021 12:45
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