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

First pass at styling the Price Plan selection component #2

Merged
merged 9 commits into from
Nov 8, 2021

Conversation

salemhilal
Copy link
Contributor

What's this?

This is some initial work to start the implementation and styling of a price plan component, and to use it in the price plan picker. It's getting mock data from the API right now. The top-level component is a flexible-width container that allows the plans inside of it to be scrolled horizontally.

All styling is done through a single CSS file, and all class names are semantic (i.e. it describes the thing, not how it should be styled) to make restyling easy.

Still to come:

  1. display meters
  2. allow the selection of a plan
  3. optionally accept callbacks for plan selection
  4. (optionally?) make the API call to select a plan for a customer

Screenshots

Default theme
image

Example light theme
image

Since the HTTP API returns snake_case variables (I Think™), we'll need
to be ok with using them as properties around the codebase. Variables
are still verboten.

I also disabled the camelcase rule from typescript-eslint, since
naming-convention is a superset of that rule.
We'll use numeral for number formatting, since they'll do it a lot better than we will (gzipped bundle size is still quite small)
This adds a stylesheet, makes the story show three (identical for now) price plans, and adds a few classes to the PlanPicker components to start getting things styled
This would demonstrate how customization might happen
Copy link
Contributor

@zeppelinnn zeppelinnn left a comment

Choose a reason for hiding this comment

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

i wonder if there's a good solution for keeping our utils synced up. Since this isn't a monorepo seems kind of challenging... 🤔

left a few comments - mostly nits. otherwise lgtm!

@@ -0,0 +1,80 @@
.octane-component {
/* Dark theme */
--octane-bg-primary: #071420;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: probably could just omit the octane in the naming, maybe just --oc-bg-primary? wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spelled out octane just to make it really obvious for vendors (like this is octane stuff, not OpenCollective or any other company's stuff)

--octane-border-primary: #c7dcd5;
--octane-fg-accent: #128475; */

--octane-spacing: 6px;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: personal thoughts are we should try match the default chakra spacing which is 4px. That way there's more continuity if we want to copy over anything from our octane-portal repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I usually pick 6 as a base unit because it can be split into both 2 and 3 in whole units. So anything that's a multiple of the spacing can be divided up without worrying about splitting pixels as often. If you feel strongly about 4 that's fine too.

}

.octane-component .label {
font-size: 0.8rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you make font-size a variable too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, totally.

src/components/PlanPicker.css Outdated Show resolved Hide resolved
src/components/PlanPicker.css Show resolved Hide resolved
}

.octane-component.price-plan-picker .price-plan-card {
display: inline-block;
Copy link
Contributor

Choose a reason for hiding this comment

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

thoughts on using flex here? considering the number of plans is dynamic it seems like flex may be a good option here, but curious to hear your thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used inline-block so that we could have a set width for all the plans to be, which would allow overflows to scroll horizontally (imagine if these end up on a phone, for example). I also like that this lets us focus on styling one dimension of plan card. We could totally make the plans adjustable in width too.For now, though, I'm gonna stick with inline-block for now just since it's generalizable, but I'm not mad at flex.

<div className='price-plan-card'>
<div className='plan-name'>{displayName} </div>
<div className='base-price'>
<span className='label starting-at'>Starting at </span>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: starting-at seems unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to give everything a unique way to be selected. The CSS file I've added is meant to be a guide / default, but I wanted it to be easy to write your own.

Copy link
Contributor

@zeppelinnn zeppelinnn Nov 8, 2021

Choose a reason for hiding this comment

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

makes sense. Do you feel like composing the container of these components with a generic flex element would oppose that thought (effectively a equivalent of a chakra <Box> wrapper)? Long as everything is labeled with classNames it seems like it would still be just as customizable, but have flex as a default

@salemhilal
Copy link
Contributor Author

@zeppelinnn I feel you on repeated utils. My only other thought is pulling out common stuff that we use internally into its own package and sharing that between the portal and this repo, but I feel like that can happen when we have more shared deps.

@salemhilal
Copy link
Contributor Author

@zeppelinnn wrt sharing code, I have an incoming PR to share types based on the openapi schema file incoming.

@salemhilal salemhilal merged commit decc468 into main Nov 8, 2021
@salemhilal salemhilal deleted the salem/price-plan-first-styling branch November 8, 2021 18:20
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.

2 participants