-
Notifications
You must be signed in to change notification settings - Fork 29
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
Consolidate aside implementation & add tip type #79
base: main
Are you sure you want to change the base?
Consolidate aside implementation & add tip type #79
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Left some comments. Generally, try to improve things along the way, the task is rarely just copy-paste 🙂 Let me know if there are questions.
shortcodes/Aside/index.js
Outdated
const utilities = { | ||
main: '', | ||
title: '', | ||
icon: '', | ||
body: '', | ||
}; |
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.
Refactor the styles in a way that's not needed anymore. Basically all styles this widget needs should be in styles - they shouldn't rely on anything either web.dev or d.c.c brings or alters. See the BrowserCompat shortcode for reference.
For example you are using --color-blue-darkest
, which is undefined for web.dev.
Define basic styling here and make it configurable using overwritable variables.
@@ -0,0 +1,100 @@ | |||
.aside { |
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 comment above for styles, this seems like a copy from web.dev.
Also please have another look at the icons. The GitHub file overview gives a good idea of what's worth to double check: highlighter and ink-highlighter are the same, the same for lightbulb and lightbulb-outline, graph looks weirdly vertically aligned, update has a weird view box. |
One last note: please make sure to sign the CLA. |
…ions_in_webdev-infra_implement_tip_type
Fixes #77