-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
3dd5b2e
to
8159f6d
Compare
7ef5776
to
7a64c49
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few notes and ideas
@@ -302,18 +302,21 @@ enum AppAction: Hashable { | |||
case mergeEntry(parent: Slashlink, child: Slashlink) | |||
case moveEntry(from: Slashlink, to: Slashlink) | |||
case updateAudience(address: Slashlink, audience: Audience) | |||
case assignColor(addess: Slashlink, color: NoteColor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would recommend just passing Color
at the level of actions. NoteColor
can act as a constant we use when assigning colors.
Rationale: the plumbing then supports any color. We enforce defaults at the io boundary. This would allow users to hack the color headers, for example.
If the intent is to support dynamically adaptive colors for light/dark, I believe SwiftUI supports this through the asset bundle (specify light/dark variants of color constants).
Alternatively, if we wanted to keep this in code, we might create a more general type like AdaptiveColor
:
struct AdaptiveColor: Hashable {
var light: Color
var dark: Color
func color(for scheme: ColorScheme) -> Color {
switch scheme {
case .light:
return light
case .dark:
return dark
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the colors into the asset bundle, see response here #1064 (comment)
@@ -119,6 +119,8 @@ enum DeckAction: Hashable { | |||
case succeedMergeEntry(parent: Slashlink, child: Slashlink) | |||
case requestUpdateAudience(_ address: Slashlink, _ audience: Audience) | |||
case succeedUpdateAudience(_ receipt: MoveReceipt) | |||
case requestAssignNoteColor(_ address: Slashlink, _ color: NoteColor) | |||
case succeedAssignNoteColor(_ address: Slashlink, _ color: NoteColor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would recommend just passing Color at the level of actions. NoteColor can act as a constant we use when assigning colors.
case requestUpdateAudience(address: Slashlink, audience: Audience) | ||
case succeedUpdateAudience(_ receipt: MoveReceipt) | ||
case requestAssignNoteColor(_ address: Slashlink, _ color: NoteColor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend passing Color in actions
@@ -610,8 +626,11 @@ struct MemoEditorDetailModel: ModelProtocol { | |||
modified: Date.distantPast, | |||
fileExtension: ContentType.subtext.fileExtension | |||
) | |||
var color: NoteColor? = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend storing Color
@@ -14,6 +14,7 @@ enum HeaderName: String { | |||
case created = "Created" | |||
case modified = "Modified" | |||
case fileExtension = "File-Extension" | |||
case color = "Color" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the header name should be Theme-Color
and the case name themeColor
Prior art: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/meta/name/theme-color
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the header should store constant names. Instead, we should consider storing hex values.
Should we include support for color variants?
Theme-Color: #abc123
Theme-Color-Dark: #abc123
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the header name, see #1064 (comment) RE: storing hex.
RE: #1064 (comment) @gordonbrander I think it's a bit more complicated than this. For each of the five note variants we have four colours:
I can see several issues with storing literal color values:
I personally think of If we want to support user-defined custom colors per-note right now I think we should consider a I moved all the card colours into the asset bundle, renamed |
|
||
public extension Color { | ||
static var cardThemeColorA: Color { | ||
Color("CardThemeColorA") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a new syntax Color(.cardThemeColorA)
on XCode 15+ but we need these string names for XCode 14.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RE: #1064 (comment)
@gordonbrander I think it's a bit more complicated than this. For each of the five note variants we have four colours:
- Light BG
- Light Highlight
- Dark BG
- Dark Highlight
I can see several issues with storing literal color values:
- schema lock-in: storing 4 values seems hefty but also locks us in to how color presents itself going forward
- color drift, if we change the defaults slightly the old colors will persist in the remaining notes
- injecting malicious colors into another user's deck
- color space, on web vs iOS we are in different color spaces and may want to tailor colors based on that
- accessibility, it makes high contrast and color-blind friendly modes difficult to implement/make guarantees around
I personally think of
NoteColor
here more like a simple bucket A, B, C, D or E that I can sort a note into, and perhaps we should use actually generic letter labels instead of colors as the cases ofNoteColor
? The concern of how that bucket is translated to onscreen pixels feels like a view-level responsibility to me. This would allow us to "theme" the whole app and re-define the colors for A...E at will, users could even customize the colors for themselves.If we want to support user-defined custom colors per-note right now I think we should consider a
NoteColor.custom(String)
case that the client can choose to support to varying degrees.I moved all the card colours into the asset bundle, renamed
NoteColor
toThemeColor
and changed the labels to generic letters as-per the above.
Ok, this makes sense for now, although I suspect we may want to consider how to handle custom color values over the long term.
Since these themes are an app-specific concern, I have one more request: let's change the header key to Subconscious-Theme
. The value can be a unique key that corresponds to the theme (a
, b
, c
, ...).
We may want to use numbers rather than letters for the theme keys to avoid uppercase/lowercase ambiguity.
Otherwise LGTM
# Conflicts: # xcode/Subconscious/Shared/Components/Deck/CardStackView.swift
Refresh views when color assigned
881d9aa
to
992a337
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🥳
Fixes #1061
WellKnownHeaders
This change dials back the use of color in the editor/viewer in anticipation of the next chunk of work: designing the mini-editor & full editor using card styles.
Screen.Recording.2024-01-09.at.12.27.13.pm.mov