-
Notifications
You must be signed in to change notification settings - Fork 1
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
implement header component #15
Conversation
63387a2
to
8a04ed1
Compare
.gitignore
Outdated
@@ -2,3 +2,4 @@ node_modules | |||
npm-debug.log | |||
.idea/ | |||
.DS_Store | |||
yarn.lock |
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.
Why? I'd rather not hide the "mistakes" (and using yarn is a mistake :) )
@@ -21,9 +21,11 @@ | |||
}, | |||
"homepage": "https://github.com/21-23/cssqd-ui#readme", | |||
"dependencies": { | |||
"preact": "^8.2.5" | |||
"preact": "^8.2.5", | |||
"react-fontawesome": "^1.6.1" |
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.
Not critical for now, but just curious: can we engage it with 21-23/_qd-ui#42 ?
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'd say addressing 21-23/_qd-ui#42 will require too much effort, so let's postpone it
<Header | ||
productName={text('productName', 'CSS Quickdraw')} | ||
username={text('username', 'username')} | ||
style={{ |
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.
Perhaps background could be set in the component style, so that Header
s are configured with a bgUrl
prop. On the other hand, there's apparent semantic distinction between "style" and "data props". Still there is no default background (and it doesn't make sense to have one), background is not optional so I wouldn't say it's styling or customization... 🤔
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.
We can try to make background b&w image and customize color with CSS (have no idea on top of my head right now how to do this)
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 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.
https://developer.mozilla.org/en-US/docs/Web/CSS/background-blend-mode - no IE support :(
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'd drop IE support completely...
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.
Multiple backgrounds then maybe
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.
Just to clarify, are you guys ok with style prop being passed down to header or you think bgUrl
prop will be more verbose? I'd say having ability to passstyle
object has a benefit as you can override several background related properties alongside with changing backgroundUrl. As an example you might want to change background position/cover/attachment with different images
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’d go with more strict ‘bgUrl’. Right now there’re no cases for overriding smth more than image
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.
see a31c3b3
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.
yay
9e38629
to
d7174e3
Compare
Closes #2