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

Feat: New Feature Popover #7302

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Feat: New Feature Popover #7302

wants to merge 12 commits into from

Conversation

cadeban
Copy link
Contributor

@cadeban cadeban commented Oct 9, 2024

Description

AVO-554: We have a lot of planned changes for our app, and we need a way to let people know about all of our cool new features.

Changes:

  • Adds New Feature Popover component
    • Only supports 1 popover at a time
    • This component supports a portal version and a non-portal version
  • Adds popover content and atomsWithStorage for Date Picker feature (not finalized)

Todo:

  • Dark Mode 🕶️
  • more tests
  • storybook

Open for discussion:

  • How do we want to handle multiple? Changelog, walkthrough (with max # of popovers)? -> just 1
  • Do we want modal background? -> probably not

Preview

Date Picker example (portal):

Desktop view Mobile view
Screenshot 2024-10-09 at 11 04 10 Screenshot 2024-10-09 at 11 04 37

Future Price example (non-portal):

Desktop view Mobile view*
Screenshot 2024-10-09 at 11 41 08 Screenshot 2024-10-09 at 11 41 37

*Note how the in the non-portal version the popover is behind the TimeController

Dark mode:
Screenshot 2024-10-11 at 16 26 33

Double check

  • I have tested my parser changes locally with poetry run test_parser "zone_key"
  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

@cadeban cadeban self-assigned this Oct 9, 2024
@github-actions github-actions bot added frontend 🎨 dependencies Pull requests that update a dependency file labels Oct 9, 2024
@cadeban
Copy link
Contributor Author

cadeban commented Oct 14, 2024

@tonypls This PR still needs some tests, but you can take a look at the code whenever you have time

- adds new feature component
- adds content for data picker popover
- adds content for new price popover
Copy link
Contributor

@silkeholmebonnen silkeholmebonnen left a comment

Choose a reason for hiding this comment

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

Looks really good! Just a couple of questions and comments

Comment on lines 55 to 58
"new-feature-popover": {
"title": "Select a Date to Explore",
"content": "Use the date picker to view historical data on electricity sources, CO2 emissions, and prices."
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should add another layer in this if we will add more feature popovers in the future?
Maybe something like this:

Suggested change
"new-feature-popover": {
"title": "Select a Date to Explore",
"content": "Use the date picker to view historical data on electricity sources, CO2 emissions, and prices."
},
"new-feature-popover": {
"date-picker": {
"title": "Select a Date to Explore,"
"content": "Use the date picker to view historical data on electricity sources, CO2 emissions, and prices."
},
},

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 think we're only adding a single new feature popover for now, since we don't want to overwhelm the user with a bunch of popovers when they first arrive

Comment on lines 26 to 30
<NewFeaturePopover
side="top"
content={<NewFeaturePopoverContent />}
portal={false}
>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be showing until we actually implement it. Meanwhile, should we just add it in a FF or what do you think?

Also, do you think we should add an end date for when it should not show up for any users anymore? e.g a month after the release of the new feature?

Comment on lines 16 to 18
const [isLoadingMap] = useAtom(loadingMapAtom);

const allLoaded = !isLoadingMap && !isLoadingData;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed when it wasn't before?

If it is needed, then we could use useAtomValue to match the other atom values in the component

Copy link
Contributor Author

@cadeban cadeban Nov 25, 2024

Choose a reason for hiding this comment

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

At the time I originally wrote this, the popover was displaying over the loading screen. However, that no longer seems to be an issue, so I will remove the isLoadingMap check

EDIT: I think it's because I removed portal as the default behavior on the popover

Comment on lines +11 to +13
<h3>{t(`new-feature-popover.title`)}</h3>
</div>
<p>{t(`new-feature-popover.content`)}</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on how we decide to do the translation (see en.json comment), then it might be necessary to give the translation strings as props

@cadeban cadeban requested a review from a team as a code owner November 26, 2024 08:27
@cadeban
Copy link
Contributor Author

cadeban commented Nov 28, 2024

Will make a follow up PR to automatically dismiss the popover if someone clicks elsewhere on the app

@cadeban
Copy link
Contributor Author

cadeban commented Nov 28, 2024

@silkeholmebonnen @tonypls would y'all please take another look at this PR? Thanks!

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 8 changed files in this pull request and generated no suggestions.

Files not reviewed (2)
  • web/package.json: Language not supported
  • web/src/locales/en.json: Language not supported
@VIKTORVAV99
Copy link
Member

@cadeban to fix the lockfile just merge master and run pnpm install before committing it to git. PNPM has built in merge resolution for the lockfiles :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file frontend 🎨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants