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

Get width and height from JavaScript evaluation. #30

Merged
merged 9 commits into from
Nov 20, 2020

Conversation

jleonard-r7
Copy link
Contributor

Fixes #29

@oliyh
Copy link
Owner

oliyh commented Nov 13, 2020

Hi,

Thanks for this! I would like to keep kamera.core agnostic of devcards, so this could better be exposed as a key in the :resize-to-contents options map (called :dom-selector or something, defaulting to body in core but overridden to the devcards selector in kamera.devcards). This would ensure users could retain the same behaviour if they wished as well.

@oliyh
Copy link
Owner

oliyh commented Nov 13, 2020

This is also something I would like to test on my projects to see what difference it would make

@jleonard-r7
Copy link
Contributor Author

Makes sense. Ok, I will add the :dom-selector change to this PR and update accordingly.

@oliyh
Copy link
Owner

oliyh commented Nov 14, 2020

Sorry I don't think I made myself clear, I think the :dom-selector should be inside the :resize-to-contents map, e.g. {:resize-to-contents {:width? true :height? true :dom-selector "body"}} as they all relate to the same feature (keeps the fn signatures cleaner too)

@jleonard-r7
Copy link
Contributor Author

Ah I agree. That makes sense. Will update later.

src/clj/kamera/devcards.clj Outdated Show resolved Hide resolved
@jleonard-r7
Copy link
Contributor Author

Is this ready to merge now?

@oliyh
Copy link
Owner

oliyh commented Nov 16, 2020

Hi,

Thanks for the updates. I am just going to try this build with my existing projects to see what happens as I am curious about how the javascript size and devcards selector will behave, and also to see what existing users can expect when they upgrade. I'll try to do this tomorrow.

Cheers

@jleonard-r7
Copy link
Contributor Author

Ok, with default settings, the only change they should see would be that the bug is actually fixed; i.e., the screenshots will be sized to content in the vertical direction. That will likely break tests where their content exceeded the vertical extent of one page (but that's most likely a good thing). They will either need to update their reference images or change the config to disable sizing to content in the vertical direction.

Everything else should be as expected / normal / least surprise.

@oliyh oliyh merged commit 5dddd18 into oliyh:master Nov 20, 2020
jleonard-r7 added a commit to jleonard-r7/kamera that referenced this pull request Nov 21, 2020
* Get width and height from JavaScript evaluation
* New option to choose which dom element to size to
Fixes oliyh#29

Co-authored-by: Jonathan Leonard <johanatan@gmail.com>
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.

:resize-to-contents seems to have no effect
3 participants