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

Consolidate aside implementation & add tip type #79

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

ShayPunter
Copy link
Collaborator

Fixes #77

@google-cla
Copy link

google-cla bot commented Apr 19, 2023

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.

@ShayPunter ShayPunter changed the title Consolidate aside implementation Consolidate aside implementation & add tip type Apr 19, 2023
@ShayPunter ShayPunter marked this pull request as ready for review April 24, 2023 09:33
Copy link
Collaborator

@matthiasrohmer matthiasrohmer left a 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 Show resolved Hide resolved
shortcodes/Aside/index.js Outdated Show resolved Hide resolved
shortcodes/Aside/index.js Outdated Show resolved Hide resolved
Comment on lines 45 to 50
const utilities = {
main: '',
title: '',
icon: '',
body: '',
};
Copy link
Collaborator

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.

shortcodes/Aside/index.js Outdated Show resolved Hide resolved
@@ -0,0 +1,100 @@
.aside {
Copy link
Collaborator

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.

@matthiasrohmer
Copy link
Collaborator

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.

@matthiasrohmer
Copy link
Collaborator

One last note: please make sure to sign the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate and consolidate aside implementations in webdev-infra, implement Tip type
2 participants