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

CED-721 Add EsCta component #1039

Merged
merged 31 commits into from
Jul 26, 2023
Merged

CED-721 Add EsCta component #1039

merged 31 commits into from
Jul 26, 2023

Conversation

jsieving
Copy link
Contributor

@jsieving jsieving commented Jul 11, 2023

πŸ”— Linked issue

https://energysage.atlassian.net/browse/CED-721

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

  • Created new EsCta component which can be customized for the different use cases described in the ticket.
    • Right now, the component only works on the demo page. It's also copied over to a component Vue file, but there are some errors in building it.
  • Updated the documentation with all the attributes of the component
    • Once I can get the component file working, I will update with more examples that reflect each use case shown in the ticket.

Feedback requested/issues:

  • @RyanMulready these seem like environment/package issues:
    • Importing from @energysage/es-vue-base in EsCta.vue doesn't work (it says the module path can't be resolved.) Importing from ../.. gets rid of warnings in VSCode, but also fails to build.
    • Container queries don't seem to work in EsCta.vue even though they work in es-cta.vue. I also see scss warnings in the code that the container-* properties are unknown.
  • For the link - @tomleo you usually have good ideas about forms:
    • When showForm is false, I use the whole card as a link. Is that bad, because it's putting block components in an inline component? Should I use @click instead and change the window.location.href?
    • When showForm is true but showZipEntry is false, clicking the button takes you to the destination URL with a "?" appended. Is this bad, and should I find some way to prevent that when the form submits?

πŸ₯Ό Testing

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.
  • I have documented testing approach

@swarmia
Copy link

swarmia bot commented Jul 11, 2023

βœ… Β Linked to Task CED-721 Β· EsVerticalCTA Component
➑️  Part of Epic CED-696 · Article Page v1 [high confidence]

Copy link
Member

@tomleo tomleo left a comment

Choose a reason for hiding this comment

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

I took a quick pass at things, overall looks good. I'll take a deeper dive into things a little later.

es-design-system/pages/molecules/es-cta.vue Outdated Show resolved Hide resolved
es-design-system/pages/molecules/es-cta.vue Outdated Show resolved Hide resolved
es-design-system/pages/molecules/es-cta.vue Outdated Show resolved Hide resolved
es-design-system/pages/molecules/es-cta.vue Outdated Show resolved Hide resolved
es-design-system/pages/molecules/es-cta.vue Outdated Show resolved Hide resolved
es-design-system/pages/molecules/es-cta.vue Outdated Show resolved Hide resolved
es-vue-base/src/lib-components/EsCta.vue Outdated Show resolved Hide resolved
es-vue-base/src/lib-components/EsCta.vue Outdated Show resolved Hide resolved
es-vue-base/src/lib-components/EsCta.vue Outdated Show resolved Hide resolved
es-vue-base/src/lib-components/EsCta.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@RyanMulready RyanMulready left a comment

Choose a reason for hiding this comment

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

@RyanMulready these seem like environment/package issues:
Importing from @energysage/es-vue-base in EsCta.vue doesn't work (it says the module path can't be resolved.)

When inside es-vue-base self imports should be treated like any other project:

import {
    formMixins,
} from '../lib-mixins';
import {
    vuelidateMinLength,
    vuelidateMaxLength,
    vuelidateNumeric,
    vuelidateRequired,
} from '../lib-validators';

Container queries don't seem to work in EsCta.vue even though they work in es-cta.vue. I also see scss warnings in the code that the container-* properties are unknown.

I don't see any errors/warnings. Can you show me what they say and/or maybe point at a specific container style that isn't working as expected?

@jsieving
Copy link
Contributor Author

@RyanMulready, thanks, I had a brain blip with the imports.
This is what I mean about warnings (I see the same thing in EsCta.vue and es-cta.vue):
Screenshot 2023-07-13 at 3 01 13 PM
It might just be my editor config being screwed up, but I thought I should point it out since the container queries weren't working on EsCta.vue.

Here's what I'm talking about (click to expand). The PR has been updated to demo this as well.
Screenshot 2023-07-13 at 3 36 34 PMScreenshot 2023-07-13 at 3 36 51 PMScreenshot 2023-07-13 at 3 37 03 PM

@RyanMulready
Copy link
Contributor

RyanMulready commented Jul 14, 2023

@RyanMulready, thanks, I had a brain blip with the imports. This is what I mean about warnings (I see the same thing in EsCta.vue and es-cta.vue): Screenshot 2023-07-13 at 3 01 13 PM It might just be my editor config being screwed up, but I thought I should point it out since the container queries weren't working on EsCta.vue.

Here's what I'm talking about (click to expand). The PR has been updated to demo this as well. Screenshot 2023-07-13 at 3 36 34 PMScreenshot 2023-07-13 at 3 36 51 PMScreenshot 2023-07-13 at 3 37 03 PM

I do not see those warnings myself. I haven't actually used containers yet, they are relatively new. However I think you might be using @container wrong? Looking at the docs I think something like this might be what you're after. Note I am defining what CSS selector I want to affect inside @container.

@container card (min-width: 600px) {
    .flex-layout-outer {
        display: flex;
    }
}

@ericdouglaspratt ericdouglaspratt self-requested a review July 17, 2023 16:29
Copy link
Collaborator

@ericdouglaspratt ericdouglaspratt left a comment

Choose a reason for hiding this comment

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

I'd like to make a few suggestions that will hopefully simplify things and make this easier to reason about:

  • EsCta should probably be an organism rather than a molecule, because it's made up of a bunch of molecules.
  • I recognize Ryan specified the props a certain way in the ticket, however I feel pretty strongly that we should pull the zip code form functionality out into its own organism component, EsZipCodeForm (very similar to the PageZipCodeForm component in es-cms-pages). We can still do that as part of this ticket and PR.
    • EsCta can then take a <slot name="cta" /> and on the docs page, we can show a few examples of what you could pass in there - either nothing (for case of the cross-sell card), or a button, or EsZipCodeForm.
    • This would greatly reduce the number of props coming into this component, which would reduce the risk of unexpected interactions between them (e.g. showForm being true but showZipEntry being false), as well as make the zip code form logic easier to reason about.
  • I think we should also separate the full-width banners from the card-like ones that will always live within a constrained column width on a page.
    • I think it'll be too difficult to ensure the layout works if we mix these two use cases into one component. The full-width banners never have an image or supporting text, so making the side-by-side layout work in cases where people happen to add those in is a waste of time. The cards never change layout - their contents are always vertically stacked, and their parent layouts will set them up in a column grid layout so they never grow too wide.
    • This may mean we don't need container queries anymore - sounds like they were difficult to get working correctly anyway?
    • The card one could be called something like EsCtaCard, and the full-page-width one could be called EsCtaBanner.
  • The image should probably be implemented as a <slot name="image"> so we can use StoryblokImage if we need to. It could still have a default <b-img> inside it and take props like imageSrc and imageAlt like EsSupportCard does, but we'll probably end up passing StoryblokImage into the slot in all cases anyway to avoid CLS.

@tomleo
Copy link
Member

tomleo commented Jul 21, 2023

When showForm is false, I use the whole card as a link. Is that bad, because it's putting block components in an inline component? Should I use @click instead and change the window.location.href? -- @jsieving

In HTML5 pretty much everything can be nested inside an anchor-tag (including block-level elements), with the exception of <button> and other <a> tags.

Copy link
Member

@tomleo tomleo left a comment

Choose a reason for hiding this comment

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

Overall looks good πŸ‘

es-vue-base/src/lib-components/EsCtaBanner.vue Outdated Show resolved Hide resolved
es-design-system/pages/organisms/es-cta-card.vue Outdated Show resolved Hide resolved
Copy link
Collaborator

@ericdouglaspratt ericdouglaspratt left a comment

Choose a reason for hiding this comment

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

Simplified the components a bit, removed props I don't think we need to match the designs, converted several text props to slots, added more robust docs and examples.

@ericdouglaspratt ericdouglaspratt merged commit 3c1bfb5 into main Jul 26, 2023
1 check passed
@ericdouglaspratt ericdouglaspratt deleted the CED-721-es-vertical-cta branch July 26, 2023 12:44
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.

4 participants