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

Add GeometryReader implementation #239

Merged
merged 15 commits into from
Aug 11, 2020
Merged

Add GeometryReader implementation #239

merged 15 commits into from
Aug 11, 2020

Conversation

MaxDesiatov
Copy link
Collaborator

@MaxDesiatov MaxDesiatov commented Aug 2, 2020

This is just an empty API at the moment. I hope it can be implemented purely in the deferredBody of GeometryReader with the ResizeObserver API without requiring any tweaks in the Renderer protocol or the reconciler.

Seems like I need the domRef modifier that writes JSObjectRef to a given binding working first, as discussed in #231.

@MaxDesiatov MaxDesiatov added the SwiftUI compatibility Tokamak API differences with SwiftUI label Aug 2, 2020
@carson-katri
Copy link
Member

I can work on implementing the features outlined in that issue

@MaxDesiatov
Copy link
Collaborator Author

I started some preliminary work on that, or do you have some code already?

@carson-katri
Copy link
Member

No, I hadn’t started yet.

@MaxDesiatov
Copy link
Collaborator Author

No worries, I'll try to get some draft of that today.

@MaxDesiatov MaxDesiatov changed the base branch from main to dom-ref-modifier August 2, 2020 18:28
Base automatically changed from dom-ref-modifier to main August 2, 2020 21:01
…okamak into geometry-reader

# Conflicts:
#	Sources/TokamakCore/MountedViews/MountedCompositeView.swift
#	Sources/TokamakDemo/TokamakDemo.swift
@MaxDesiatov
Copy link
Collaborator Author

This is working for me now in isolated example code, but layout bugs exposed by our demo code prevent this from being added to the demo. @carson-katri @j-f1 lmk if you think it would be fine merging this as is and adding a GeometryReader demo in a separate PR (smaller PRs is what I prefer personally).

@MaxDesiatov MaxDesiatov marked this pull request as ready for review August 4, 2020 09:48
@j-f1
Copy link
Member

j-f1 commented Aug 4, 2020

IMO if it's a small bug or something that prevents building it shouldn't be merged in general. But if it's more complex or harder-to-solve bug, it's ok to merge then follow up with an issue or PR.

@carson-katri
Copy link
Member

What's the layout bug preventing a demo?

@MaxDesiatov
Copy link
Collaborator Author

MaxDesiatov commented Aug 4, 2020

If you launch the demo in this branch in browsers and navigate to the GeometryReader item you'll see that the size text doesn't change if you change the size of the viewport (I resized the browser window). It does change in the native demo though.

As far as I understand, the destination view of NavigationView doesn't fill all available space currently, maybe it's fixed with the _FlexFrameLayout changes made in #241?

@j-f1
Copy link
Member

j-f1 commented Aug 4, 2020

I got that to work after I added this CSS to GeometryReader:

display: flex;
align-items: center;
justify-content: center;

…weirdly, the text isn’t exactly centered.

@MaxDesiatov
Copy link
Collaborator Author

These styles worked only in Safari for me and only for height, width always stays 300. I've pushed these changes for the lack of a better option, will have a look later if #241 could resolve it.

# Conflicts:
#	Sources/TokamakCore/Views/Buttons/NavigationLink.swift
#	Sources/TokamakCore/Views/Containers/NavigationView.swift
#	Sources/TokamakCore/Views/Navigation/NavigationLink.swift
#	Sources/TokamakCore/Views/Navigation/NavigationView.swift
#	Sources/TokamakCore/Views/NavigationLink.swift
#	Sources/TokamakCore/Views/NavigationView.swift
#	Sources/TokamakStaticHTML/Resources/TokamakStyles.swift
@MaxDesiatov MaxDesiatov requested a review from j-f1 August 10, 2020 20:49
Copy link
Member

@carson-katri carson-katri left a comment

Choose a reason for hiding this comment

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

Works great!

@MaxDesiatov MaxDesiatov merged commit b7434a2 into main Aug 11, 2020
@MaxDesiatov MaxDesiatov deleted the geometry-reader branch August 11, 2020 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SwiftUI compatibility Tokamak API differences with SwiftUI
Development

Successfully merging this pull request may close these issues.

3 participants