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

Fix #2831: Auto adjust the width to fit content of a Dialog Framework #2834

Closed
wants to merge 1 commit into from

Conversation

culjar
Copy link

@culjar culjar commented Oct 18, 2017

Fix #2831 issue

@tandraschko
Copy link
Member

Could someone re-test it?

@jxmai
Copy link
Contributor

jxmai commented Oct 20, 2017

Indeed, the width calculation is

frameWidth = cfg.options.contentWidth||640;

Previously, if no width is defined or even auto, the cfg.options.contentWidth will be undefined and frameWidth will always be 640px

image


However, with this PR:

image

Here is the main point:

The width auto is correctly set, which is nice in theory, but I don't see any visual differences as the end result

image

@culjar Please explain the purpose of this PR and the use case you need to address since the end result seems the same before and after the PR.

@jxmai
Copy link
Contributor

jxmai commented Oct 20, 2017

I even tried to double the amount of columns and to see the width:auto can do anything better than 640px, but the end result is not desirable.

image

Since you need to set the width anyway to make the table above become usable, I don't see any improvement can be made by setting width:auto 🤔 .

This PR currently is +0 for me. Any other opinion is welcome 😃

Copy link
Contributor

@jxmai jxmai left a comment

Choose a reason for hiding this comment

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

Review Summary:

This PR is OK, but I don't see the improvement currently. +0 for me.

If the end result indeed no differences at all after reconfirming, then -1 for me since there are more lines of code with slightly worse performance than the previous version.

At the end of the day, the end result is all that matters.

@culjar
Copy link
Author

culjar commented Oct 20, 2017

To me, works perfectly.

Next I put several screenshots so that you see the good functioning.

Dialog framework with datatable:
view

Options for open dialog:
codebean

Result in console of Chrome (You can see that it perfectly calculates the width of the iframe):
content
iframe

@jxmai I think the problem in your case is that you are not putting a width size to the datatable and therefore is unable to calculate it, try to set a fixed width or create a dialog framework with other components that are not datatable.

This PR works perfectly and is not +0.

@jxmai
Copy link
Contributor

jxmai commented Oct 20, 2017

@culjar Could you provide a full runnable sample for us to verify again since your code in #2831 is based on part of the showcase but not complete

think the problem in your case is that you are not putting a width size to the datatable and therefore is unable to calculate it, try to set a fixed width or create a dialog framework with other components that are not datatable.

Not exactly. Please see below

image

image

@culjar
Copy link
Author

culjar commented Oct 23, 2017

I downloaded primefaces-test and I made the example of Dialog Framework that is in the Showcase and does not work the automatic adjustment of the width of the dialog with the solution that I proposed.

At first I tested it in a project of mine and it seemed to work, but I have realized that it is not like that, it also does it wrong.

You're right @jxmai . Sorry to have put an invalid solution.

I hope someone can solve it as soon as possible.

@tandraschko
Copy link
Member

So we can close that PR?

@jxmai
Copy link
Contributor

jxmai commented Oct 24, 2017

Yes. This PR cannot address the issue. I might take a look later, but this PR can be closed.

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.

None yet

3 participants