-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Streamlined theme and randomly generate colors #86
Conversation
3c88b60
to
cabb8e2
Compare
<fieldset> | ||
<legend>Queue Inbound Channel</legend> | ||
x-flex direction="column"> | ||
<h3>Queue batches of lightning channels to open in a single transaction</h3> |
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.
Placing this line here gives the correct location. Even though it probably shouldn't be in the form
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 It's good if the form has a header that says what it's for ✅
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.
Nice!!!
to bottom right, | ||
rgb(${color_triplet.join(", ")}), | ||
rgb(7, 10, 19) | ||
)`; |
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.
Nit: indent to the previous line
rgba(208, 255, 0, 0.986) | ||
); | ||
color: transparent; | ||
color: var(--primary); |
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 it's an anti-pattern to style top-level typography with --primary and --secondary. Those are your theme colors. If they were colored within a context, or specifically via style attributes for emphasis that would make more sense. they're already themed with color: var(--color)
.
I'm inclined to let it pass because it's not a regression, but it's a step sideways in terms of keeping a neat theme.
|
||
input { | ||
text-align: right; | ||
} | ||
|
||
legend { | ||
color: var(--secondary); |
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.
same comment as p, h1, h3 re: color: var(--color);
.colored-button { | ||
color: black; | ||
background: var(--secondary); | ||
} | ||
|
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.
You missed the checkbox by styling this with a custom class. When you inspect the buttons you applied this too, you can see they're already styled with --buttonColor
and --buttonBg
, which is the place for this change. (--buttonColor: var(--secondary);
)
As-is this change is a regression
This gets the job done |
Instead of xmas #62,
Random color theme engaging, try it out