-
Notifications
You must be signed in to change notification settings - Fork 0
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
New state-specific calculator design #20
Conversation
## Description This implements most of the spec'd design. There are some significant pieces still missing. They're tracked in Asana, but just as a summary: - Multiple selection of projects. - Adding authority logos to the bottom of the page. - Handling the case of entering a location outside RI. - Adding an optional email field for people to join the mailing list. In addition, there are some more pieces missing before this frontend is ready to replace the existing IRA calculator wholesale: - Spanish localization - Analytics - Appearance customizability via CSS ### Guide to new code - New .html page for the RI calculator, which pretty much just has the embed. It's included in the Parcel targets, which means it will be built and deployed. It's not ready for the public yet, so it uses a meta tag to disable search engine indexing. - `state-calculator.ts` is the custom HTML element. This contains all the state management. - `state-incentive-details.ts` is the view layer. - A couple of sub-parts are factored out: `utility-selector.ts` and `icon-tab-bar.ts`. - `calculator-types-v1.ts` corresponds to the shape of the API responses. This is manually maintained right now, not generated from the API in any way; eventually it'd be good to change that. - `projects.ts` has icons and labels for the various projects. ## Test Plan - Enter a RI zip code. 02807 for Block Island, 02861 for Providence (RI Energy only), 02814 for Pascoag (RIE and PUD available). - Fill in the rest of the form, click Calculate. Look at results. In the 02814 case, switch to the other utility and make sure the incentives reload. - Click the pill buttons to switch projects. - Click the "Learn more" / "Visit site" buttons. Resize the window to look at the three possible layouts. (1, 2, or 3 columns of incentive cards.)
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
I'm sorry, I know this PR is enormous, but (a) there was a lot of churn in getting it to this state, so I'm glad I wasn't sending out smaller PRs as I went through that; and (b) I don't think it's worth breaking this PR up into smaller ones. I tried to sort out the history of the original branch, but it was just too tangled, and it would have taken a lot of time we don't really have. |
src/calculator.ts
Outdated
@@ -150,12 +150,14 @@ export class RewiringAmericaCalculator extends LitElement { | |||
? nothing | |||
: formTemplate( | |||
[ | |||
'', |
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.
I had to double-check what this was - might help to use a constant for NO_PROJECT, or a comment to clarify that this value isn't used for this version?
w: number = 20, | ||
h: number = 20, | ||
) => | ||
svg`<svg width="${w}" height="${h}" viewBox="0 0 20 20" fill="none" 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.
For future consideration, would not block this PR.
I think it might help to organize these svgs as files and use parcel to import them? They'd still be inlined in the resulting build, which I think is right for this use-case since they'll all be displayed (though if they were referenced as img
tags instead I think they'd be loaded after the main script, which might also be OK?)
A possible downside is that we'd lose the ability to control the width and height directly in the markup, but I think you can solve that with css if needed. Similarly I think the selection color might be possible to set with css and maybe currentColor
(which should be picked up from the prevailing text color already)?
The syntax is:
import svg from 'bundle-text:./logo.svg';
See inlining as a string - https://parceljs.org/languages/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.
(If we do move these to files, my thinking is that they'd be easier to manage and update (and preview) in a text editor, and also that this file would be easier to navigate and understand.)
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.
I looked into this a bit and didn't get it right on the first try, so I'll defer it to another PR.
query.append('authority_types', 'federal'); | ||
query.append('authority_types', 'state'); | ||
query.append('authority_types', 'utility'); | ||
query.set('utility', this.utility); |
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.
When this is blank do we just use our best guess? I wonder if that would be better as an explicit parameter?
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 is going to change soon, as I get the non-RI experience implemented. As it is, this code won't execute until this.utility
is set; the above _utilitiesTask
, upon success, sets this.utility
and runs this task.
To be clear, these tasks are invoked imperatively, not in response to property changes as in the existing embed implementation. It was much easier to get working this way, given the need to respond to the utility selector changing, which is outside of the main form.
src/icon-tab-bar.ts
Outdated
align-items: center; | ||
justify-content: center; | ||
|
||
& .caption { |
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.
Does this use CSS nesting or is there a build step?
I think it's probably/unfortunately too early for CSS nesting without a polyfill? https://caniuse.com/css-nesting
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.
No, this is just nesting that arrives in the browser as-is. This is why I was musing about SCSS a while ago.
I'll convert this to BEM style for now, but I would somewhat rather switch this codebase to another CSS solution, probably either SCSS/similar or Tailwind. (Possible there's something else better, too. I'm not a CSS expert; I know just enough to be dangerous.)
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.
I would somewhat rather switch this codebase to another CSS solution, probably either SCSS/similar or Tailwind.
I'd support us switching to Tailwind. It should allow us to more quickly lift over component and layout styles from the main site(s), as well as share design tokens by configuring the build the same way. And I think it only builds what you use, so it shouldn't be huge.
That said, SCSS or another preprocessor would be fine too.
This looks good to me! Couple of minor bits of feedback above. The only blocking feedback is to ensure that this builds as a standalone js file before merging. I don't think the rest are blocking, but I would like to follow up on the CSS nesting question as well as some of the file organization questions:
I verified that the behavior of the old calculator is unaffected by this PR. (Which I think the cypress test also confirms) One note on interaction: I think we need loading feedback on the map/title and the utility form as well, and maybe on the calculate button. The load spinner was below the fold for me and I couldn't tell if it was doing anything when I changed the form inputs. Made a note on https://app.asana.com/0/1205057814651000/1205391730235898/f Last thought - I was wondering if the Rhode Island HTML page should have the same developer-facing intro as the index page. We can probably hold off on that until we're ready to deprecate (or sunset) the old one. |
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.
Approved once we have state-calculator.js
building standalone. I'd like to quickly try that on https://github.com/rewiringamerica/consumer/pull/402 before merging - feel free to take over that PR if I'm not available to do that myself!
I tried this on your |
Also, final note: this will become publicly accessible on |
Updated https://github.com/rewiringamerica/consumer/pull/402 to confirm this all works. Great milestone! |
Description
This implements most of the spec'd design. There are some significant
pieces still missing. They're tracked in Asana, but just as a summary:
Multiple selection of projects.
Adding authority logos to the bottom of the page.
Handling the case of entering a location outside RI.
Adding an optional email field for people to join the mailing list.
In addition, there are some more pieces missing before this frontend
is ready to replace the existing IRA calculator wholesale:
Spanish localization
Analytics
Appearance customizability via CSS
Guide to new code
New .html page for the RI calculator, which pretty much just has the
embed. It's included in the Parcel targets, which means it will be
built and deployed. It's not ready for the public yet, so it uses a
meta tag to disable search engine indexing.
state-calculator.ts
is the custom HTML element. This contains allthe state management.
state-incentive-details.ts
is the view layer.utility-selector.ts
andicon-tab-bar.ts
.calculator-types-v1.ts
corresponds to the shape of the APIresponses. This is manually maintained right now, not generated
from the API in any way; eventually it'd be good to change that.
projects.ts
has icons and labels for the various projects.Test Plan
Enter a RI zip code. 02807 for Block Island, 02861 for Providence
(RI Energy only), 02814 for Pascoag (RIE and PUD available).
Fill in the rest of the form, click Calculate. Look at results. In
the 02814 case, switch to the other utility and make sure the
incentives reload.
Click the pill buttons to switch projects.
Click the "Learn more" / "Visit site" buttons.
Resize the window to look at the three possible layouts. (1, 2, or 3
columns of incentive cards.)