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.

Sign up for GitHub

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

Jump to bottom

Initial implementation of onHover #448

Merged
merged 1 commit into from
Sep 20, 2021
Merged

Initial implementation of onHover #448

merged 1 commit into from
Sep 20, 2021

Conversation

agg23
Copy link
Contributor

@agg23 agg23 commented Sep 18, 2021

Adds a basic implementation of action modifiers, allowing for dynamic event handling. For testing, adds a trivial onHover example.

Very simple content compared to tracing the execution path of Tokamak :P

@carson-katri carson-katri added SwiftUI compatibility Tokamak API differences with SwiftUI DOM/HTML renderer Tokamak in the browser labels Sep 19, 2021
MaxDesiatov
MaxDesiatov reviewed Sep 20, 2021
View reviewed changes
Package.resolved Outdated
@@ -1,61 +0,0 @@
{
Copy link
Collaborator

@MaxDesiatov MaxDesiatov Sep 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Please revert the removal of this file. We'd like to keep it in the repository to make sure that our dependency tree is consistent across checkouts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. My mistake.

MaxDesiatov
MaxDesiatov reviewed Sep 20, 2021
View reviewed changes
Sources/TokamakCore/Modifiers/HoverActionModifier.swift Outdated
// See the License for the specific language governing permissions and
// limitations under the License.

protocol HoverActionType {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any purpose to this protocol? It only seems to be used in the extension below, but doesn't seem to be required there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I was replicating AppearanceActionType, but that protocol is actually used elsewhere for casting, justifying its existence.

@MaxDesiatov MaxDesiatov changed the title Initial implementation of action modifiers and example onHover Initial implementation of onHover Sep 20, 2021
MaxDesiatov
MaxDesiatov reviewed Sep 20, 2021
View reviewed changes
Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall it's great to see onHover finally implemented, but this needs fixes for linter warnings and also some snapshot tests for generated HTML. Thanks!

MaxDesiatov
MaxDesiatov approved these changes Sep 20, 2021
View reviewed changes
Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I see now that this is DynamicHTML, so probably no snapshot test for this can be written at the moment. LGTM then, thanks!

@MaxDesiatov MaxDesiatov requested a review from a team September 20, 2021 15:08
@agg23
Copy link
Contributor Author

agg23 commented Sep 20, 2021

also some snapshot tests for generated HTML

I looked for existing test infrastructure when starting this PR, and I didn't find anything outside of Core and StaticHTML tests. Is there some precedent on how we want to write tests for TokamakDOM and other renderers?

@MaxDesiatov MaxDesiatov merged commit da66063 into TokamakUI:main Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxDesiatov MaxDesiatov MaxDesiatov approved these changes

Assignees
No one assigned
Labels
DOM/HTML renderer Tokamak in the browser SwiftUI compatibility Tokamak API differences with SwiftUI
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@agg23 @MaxDesiatov @carson-katri