-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
🪟 Updates to billing page icons & buttons #15204
Conversation
Hi Nataly! Thanks for taking this on. 🎉 Could you add some more detail in the PR description? It's helpful to have a sentence or so describing what the PR achieves. We usually follow the format: What(what the pr changes) How(how we did it) Reading order(optional, sometimes there's an order to read the files in so things make more sense) Sometimes we include other notes if anything else seems helpful to include. A screenshot for visual changes (ie: a before and after of where that icon was) is really helpful as well! |
@tealjulia thank you for the feedback - I added more details! |
} | ||
|
||
export const CreditsIcon = ({ color = theme.greyColor20 }: Props) => ( | ||
<svg width="30" height="24" viewBox="0 0 30 24" fill={color} xmlns="http://www.w3.org/2000/svg"> |
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.
<svg width="30" height="24" viewBox="0 0 30 24" fill={color} xmlns="http://www.w3.org/2000/svg"> | |
<svg width="30" height="24" viewBox="0 0 30 24" fill={color}> |
@@ -488,7 +488,7 @@ | |||
|
|||
"credits.credits": "Credits", | |||
"credits.whatAreCredits": "What are credits?", | |||
"credits.buyCredits": "Buy credits", | |||
"credits.buyCredits": "+ Buy credits", |
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.
@timroes currently this is how it's done but instead how about a +
svg icon or should we leave it with the existing pattern?
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.
@edmundito - @timroes mentioned that it would be nice to change it to an svg icon, but in a different PR. If there are any updates to that, I am happy to make the change though!
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.
@edmundito I would keep this as a +
for now, until we've redone the Button component and give it an option to specifcy an icon at which point I'd suggest we exchange all the existing +
into real icons.
@@ -22,16 +21,12 @@ import OnboardingIcon from "views/layout/SideBar/components/OnboardingIcon"; | |||
import SettingsIcon from "views/layout/SideBar/components/SettingsIcon"; | |||
import SidebarPopout, { Icon, Item } from "views/layout/SideBar/components/SidebarPopout"; | |||
import SourceIcon from "views/layout/SideBar/components/SourceIcon"; | |||
import { CreditsIcon } from "../../../../../components/icons/CreditsIcon"; |
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.
This suggestion should be applied locally so that the linter can sort the imports correctly.
import { CreditsIcon } from "../../../../../components/icons/CreditsIcon"; | |
import { CreditsIcon } from "components/icons/CreditsIcon"; |
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.
Code LGTM, tested locally in Chrome, everything works as expected.
What
A few small tweaks to close the first three issues in this epic:
Part of https://github.com/airbytehq/airbyte-cloud/issues/2130
On the /credits page:
Before:
After:
In the sidebar:
Before:
After:
How
Adds an SVG icon component, all other changes are simply UI changes with no logic being changed.
Reading order
en.json
RemainingCredits.tsx
CreditsIcon.tsx
SideBar.tsx