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: 1580 list favorite activities for consep #1771

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

Conversation

xiaopeng0202
Copy link
Collaborator

@xiaopeng0202 xiaopeng0202 commented Dec 9, 2024

Description

Closes #1580

Changelog

New

  • Add a new page for consep favorite activities. The behavior will look like spar favorite activities.

How was this tested?

  • 🧠 Not needed
  • 👀 Eyeball
  • 🤖 Added tests

What gif/image best describes this PR or how it makes you feel?


Thanks for the PR!

Deployments, as required, will be available below:

Please create PRs in draft mode. Mark as ready to enable:

After merge, new images are deployed in:

@xiaopeng0202 xiaopeng0202 changed the title Feat/1580 list favorite activities for consep Feat: 1580 list favorite activities for consep Dec 9, 2024
@xiaopeng0202 xiaopeng0202 changed the title Feat: 1580 list favorite activities for consep feat: 1580 list favorite activities for consep Dec 9, 2024
Copy link
Collaborator

@mgaseta mgaseta left a comment

Choose a reason for hiding this comment

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

Nice job!
Added a few markups on the code.

Not sure if it's in the scope of this task, but there is no menu entry for this consep page... I had to access the new page via the URL.

public record FavouriteActivityCreateDto(@NotNull String activity) {}
public record FavouriteActivityCreateDto(
@NotNull String activity,
@Schema(description = "Indicates whether this activity is a Consep", defaultValue = "false")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you meant: "Indicates whether this activity is from Consep" :)

ALTER TABLE
spar.favourite_activity
ADD
COLUMN is_consep BOOLEAN DEFAULT FALSE NOT NULL;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Git is complaining about a missing empty line at the end of the file.

@@ -165,4 +165,4 @@ services:
ORACLE_SYNC_USER: ${ORACLE_SYNC_USER:-proxy_fsa_spar_read_only_user}
depends_on:
database:
condition: service_started
condition: service_started
Copy link
Collaborator

Choose a reason for hiding this comment

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

Git is complaining about a missing empty line at the end of the file.

isConsep: boolean;
}

const FavouriteActivities: React.FC<FavouriteActivitiesProps> = ({ isConsep }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why using React.FC here? We normally don't use it on our "common" components...

@@ -65,11 +73,14 @@ const FavouriteActivities = () => {
description="You can favourite your most used
activities by clicking on the heart icon inside each page"
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to adjust this section accordingly with the new design for consep "empty favourites".

key={favObject.type}
favObject={favObject}
/>
) : favActQuery.data.filter((fav) => fav.isConsep === isConsep).map((favObject) => (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this filter needed here? Won't this impact SPAR's favourite activities?

}
});

const thisFavAct = favActQuery?.data?.filter((act) => act.type === activity)[0];
const thisFavAct = favActQuery?.data?.filter(
(act) => act.type === activity && act.isConsep === isConsep
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it necessary to check if the activity is from CONSEP here? Shouldn't it be enough to check the type only?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the folder name should be all in lower case, to maintain the pattern we are using. And maybe a task for later, but now that we are starting CONSEP, we can try to re-arrange our project's folder organization.

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.

CONSEP landing dashboard cards
2 participants