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 default dark styles for Views #241

Merged
merged 21 commits into from
Aug 10, 2020
Merged

Add default dark styles for Views #241

merged 21 commits into from
Aug 10, 2020

Conversation

carson-katri
Copy link
Member

@carson-katri carson-katri commented Aug 3, 2020

This provides dark mode styles for the background and many elements, such as Text, Button, TextField, etc.

Resolves #237.

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

Let me know if any of the designs/colors need to be tweaked.

@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Aug 3, 2020

Chrome and Firefox have these blue underlines for NavigationLink items for me (probably should be the same color as text if we display those at all?). In Safari 13.1.2 on Catalina there's no underline, but NavigationView destination still has white background. Also, in the OutlineGroup demo the disclosure control has very low-contrast in dark mode. Placeholder text in the TextField demo is also low-contrast in Chrome.

Screenshot 2020-08-03 at 14 01 48

Screenshot 2020-08-03 at 14 05 15

Screenshot 2020-08-03 at 14 05 24

@carson-katri
Copy link
Member Author

I've already fixed the NavigationLink color, as well as the DisclosureGroup. I'm working on a _NavigationLinkStyle for the sidebar to make it match the Big Sur styles, with the highlighted background. I'm going to try the color-scheme: dark; property to see if that fixes the buttons/textfields.

@carson-katri
Copy link
Member Author

Here's the dark scheme Views using the color-scheme property.

Screen Shot 2020-08-03 at 11 11 59 AM

Screen Shot 2020-08-03 at 11 13 08 AM

Screen Shot 2020-08-03 at 11 14 25 AM

I think the picker looks weird, but besides that these are much nicer. Except they don't work in Chrome. Is there a way for me to provide a different style for Chrome?

@j-f1
Copy link
Member

j-f1 commented Aug 3, 2020

I remember that being an issue in Chrome. I think they’re planning to have custom (non-native-looking) default form controls which would probably support dark mode.

@carson-katri
Copy link
Member Author

Ok, I've got the SidebarListStyle rendering the NavigationLink elements right. I was also able to switch NavigationView to use an ObservableObject for the context 🎉

What are your opinions on Chrome styling. Do we write in default dark mode styles?

@ie-ahm-robox
Copy link
Collaborator

Fails
🚫

Sources/TokamakStaticHTML/Views/Text/Text.swift#L127 - Function body should span 50 lines or less excluding comments and whitespace: currently spans 52 lines (function_body_length)

Generated by 🚫 Danger Swift against c0a5806

@carson-katri
Copy link
Member Author

Oh, I also had to modify the LayoutModifier to check for an infinite frame. That is:

.frame(minWidth: 0, maxWidth: .infinity)
.frame(minHeight: 0, maxHeight: .infinity)

Will always fill it’s parent on the respective axis.

@j-f1
Copy link
Member

j-f1 commented Aug 4, 2020

Great to see you have selected link highlighting working!

I’ve noticed a bug in Safari: when you load the demo, click on a section in a sidebar, switch to a different tab, then come back, the content of the NavigationView permanently disappears (the sidebar remains) and clicking other navigation links has no effect. There are no errors in the console.

@carson-katri
Copy link
Member Author

Maybe its related to the context now being an ObservableObject?

@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Aug 4, 2020

Were any of you able to reproduce #245 while testing this? I wonder if it's somehow related. Would be in line with the bugs in dark mode support in Safari that @j-f1 saw previously.

@carson-katri
Copy link
Member Author

No, I haven't been able to reproduce that..

@carson-katri
Copy link
Member Author

The NavigationView not using the entire size viewport should be fixed. All that's left is to add dark mode styles for List.

@carson-katri
Copy link
Member Author

carson-katri commented Aug 7, 2020

I fixed the color thing by putting the string directly as the innerHTML instead of putting a span inside, but I can't figure why you have to press the edge of the button. I thought the span may have been causing it, but it wasn't that.

Any ideas?

@carson-katri
Copy link
Member Author

Also is it only happening in Safari betas? If so, it may be their bug.

@j-f1
Copy link
Member

j-f1 commented Aug 7, 2020

I only see it happening in Safari but I also only happen to have beta versions of Safari installed right now so I can’t check the shipping version. I can try to report it though!

I did a quick test of a button with a span inside it, essentially copying the HTML of the Tokamak Button, and it worked fine.

@carson-katri
Copy link
Member Author

I tested on Catalina and got the same issue with the buttons.

@carson-katri
Copy link
Member Author

Ok, I fixed it by disabling:

"pointerdown": { _ in isPressed = true },
"pointerup": { _ in isPressed = false },

These events are not supported in Safari. Is there another way to get this information?

@MaxDesiatov
Copy link
Collaborator

I guess these could be mousedown and mouseup, which I initially used before @j-f1 suggested switching to pointer events

@MaxDesiatov
Copy link
Collaborator

MDN says that the pointer events spec is obsolete, I thought it was the recent one 🤔 https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/pointerdown_event

@carson-katri
Copy link
Member Author

mousedown and mouseup are also not working for me...

@j-f1
Copy link
Member

j-f1 commented Aug 9, 2020

Pointer events are supported in Safari 13. It looks like there’s a version 2 of the spec which is why the old one is obsolete.

@carson-katri carson-katri requested review from MaxDesiatov and j-f1 and removed request for j-f1 August 10, 2020 02:14
MaxDesiatov
MaxDesiatov previously approved these changes Aug 10, 2020
Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Thank you 🙌

@j-f1
Copy link
Member

j-f1 commented Aug 10, 2020

Two issues in this branch in Safari that I can’t reproduce on main:

  • clicking the counter until it should show “Limit Exceeded” instead throws an error
  • clicking the button in the DOM Reference demo does nothing or briefly shows the changed text before reverting

@carson-katri
Copy link
Member Author

I can't replicate 1, but fixed 2.

@MaxDesiatov
Copy link
Collaborator

Same, I wasn't able to replicate 1 on my side

Copy link
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

LGTM! Opened #256 for the first issue as it’s unrelated.

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.

Add dark color scheme style
4 participants