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

🪟 Updates to billing page icons & buttons #15204

Merged
merged 3 commits into from
Aug 9, 2022
Merged

Conversation

natalyjazzviolin
Copy link
Contributor

@natalyjazzviolin natalyjazzviolin commented Aug 2, 2022

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:

  1. Updates CTA button text and size. Closes https://github.com/airbytehq/airbyte-cloud/issues/2148
  2. Removes icon before credit amount. (No issue created for this one)

Before:
Screen Shot 2022-08-03 at 9 19 20 AM
After:
Screen Shot 2022-08-03 at 9 20 59 AM

In the sidebar:

  1. Updates 'credits' icon from star to a new custom icon. Closes https://github.com/airbytehq/airbyte-cloud/issues/1537

Before:
Screen Shot 2022-08-03 at 9 19 44 AM
After:
Screen Shot 2022-08-03 at 9 21 16 AM

How

Adds an SVG icon component, all other changes are simply UI changes with no logic being changed.

Reading order

  1. en.json
  2. RemainingCredits.tsx
  3. CreditsIcon.tsx
  4. SideBar.tsx

@natalyjazzviolin natalyjazzviolin requested a review from a team as a code owner August 2, 2022 15:25
@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Aug 2, 2022
@teallarson
Copy link
Contributor

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!

@timroes timroes self-requested a review August 2, 2022 18:45
@natalyjazzviolin
Copy link
Contributor Author

@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">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<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",
Copy link
Contributor

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?

Copy link
Contributor Author

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!

Copy link
Collaborator

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";
Copy link
Contributor

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.

Suggested change
import { CreditsIcon } from "../../../../../components/icons/CreditsIcon";
import { CreditsIcon } from "components/icons/CreditsIcon";

Copy link
Collaborator

@timroes timroes left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants