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

Feature/savedprops #230

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

Feature/savedprops #230

wants to merge 31 commits into from

Conversation

kailasnadh790
Copy link
Collaborator

@kailasnadh790 kailasnadh790 commented May 10, 2024

Below features are implemented:

  • List saved properties
  • Pre-fill heart icon for saved properties on property listing
  • unsave property on saved-properties page

Fix #153

Test URLs:
https://feature-savedprops--hsf-commonmoves--hlxsites.hlx.page/account/saved-properties

Copy link

aem-code-sync bot commented May 10, 2024

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

Copy link

aem-code-sync bot commented May 10, 2024

Page Scores Audits Google
M /account/saved-properties PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
D /account/saved-properties PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops May 10, 2024 16:49 Inactive
@kailasnadh790 kailasnadh790 requested a review from bstopp May 10, 2024 16:50
@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops May 10, 2024 16:51 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops May 10, 2024 16:52 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops May 14, 2024 19:10 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops May 14, 2024 19:10 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops May 21, 2024 16:18 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops May 21, 2024 16:20 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops May 21, 2024 17:10 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops May 21, 2024 18:26 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops May 21, 2024 18:26 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops May 21, 2024 18:47 Inactive
}
const { contactKey } = user;

getSavedProperties(contactKey).then((results) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this is going to work. Line 37 is a promise, so this will always have no results when it executes. If needed, this should be chained with the promise returned there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bstopp I see your point. It is somehow working without chaining. Have not moved the code inside property search promise.

@@ -445,6 +447,59 @@ main .section.banner .default-content-wrapper {
text-transform: uppercase;
}

.btn {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for this:
image
Felt these could be candidates for global primary and secondary buttons...as I didn't see such styling already implemented.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a primary button global definition, it has a specific dom structure p.button-container > a.

There isn't a secondary one, but that can be achieved with a specific dom structure as well p.button-container > em > a
This will add button secondary classes to the anchor, we would just need to add the CSS to create the ux of the button on the right.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't made this modal authorable. So, have hard-coded btn-primary and btn-secondary and defined the custom styles for them in styles.css.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, but even though this isn't authorable doesn't mean we can't still generate the same markup as the authored styles, so that we use the same, existing CSS

@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops May 23, 2024 01:20 Inactive
@kailasnadh790 kailasnadh790 requested a review from bstopp May 23, 2024 01:23
@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops May 28, 2024 22:49 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops June 5, 2024 13:50 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops June 5, 2024 14:01 Inactive
@aem-code-sync aem-code-sync bot temporarily deployed to feature/savedprops June 17, 2024 14:08 Inactive
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.

Saved Properties Results Page
2 participants