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
app(IN-941): Search page alerts #1182
base: dev
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
@@ -43,6 +43,77 @@ const ServiceFilter = dynamic(() => | |||
import('@weareinreach/ui/modals/ServiceFilter').then((mod) => mod.ServiceFilter) | |||
) | |||
|
|||
const stateRiskLevels = { |
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.
Sorry this is a bit ugly, I could do 4 arrays and then build some logic if else statements, not sure.
@@ -63,6 +63,8 @@ const colors = { | |||
coolGray: '#d9d9d9', | |||
red: '#C05C4A', | |||
pink: '#D4A1BA', | |||
lightPink: '#F2E1E9', | |||
lightGreen: '#E4F9EA', |
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 chose the light green based on this https://m2.material.io/design/color/the-color-system.html#tools-for-picking-colors
This was not in the design, so not sure if we want to use this instead of the lightPink for all states.
📦 Next.js Bundle Analysis for @weareinreach/appThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
</Text> | ||
</Box> | ||
)} | ||
|
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.
Can we extract the banner code out in to a separate component?
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.
Yes good call, I'll work on that this week!
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.
Would the banners go in the story book as components?
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.
Yeah, you can create it in components/core
in the ui package - there are plenty of *.stories.tsx
files to use as a starting template.
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.
Thanks!
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.
Moved the banners to components although my folder structure may be a bit different, was having an issue with imports
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 fixed part of it - you were trying to move the component in to a storybook file (*.stories.tsx
shouldn't get imported in to any functional part of the app - those are for Storybook only). I moved the component to ui/components/core/LocationBasedAlertBanner/index.tsx
and set up 3 different use examples. Inside the storybook file, in the RenderWrapper
component I put in there (just for purposes of pulling mock API data), you'll see how the API call is done. The API can return more than one result, in the event that there are multiple alerts for an area or overlapping alerts
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 didn't touch the implementation on this page though -- that still needs to be done
This PR currently has a merge conflict. Please resolve this and then re-add the |
Signed-off-by: Joe Karow <58997957+JoeKarow@users.noreply.github.com>
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@types/ms@0.7.34, npm/@types/node@16.18.97, npm/@types/yargs@17.0.32, npm/dotenv@16.4.5, npm/prettier@3.2.5, npm/zod@3.23.8 |
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.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/db/prisma/schema.prisma (4 hunks)
Additional comments not posted (5)
packages/db/prisma/schema.prisma (5)
781-781
: AddedLocationAlert
relation toFreeText
model.Ensure that the
LocationAlert
model is properly defined elsewhere in the schema to support this relation.
1303-1303
: AddedLocationAlert
relation toCountry
model.Verify that the
LocationAlert
model is correctly implemented and that the relation is consistent with the application's requirements.
1356-1356
: AddedLocationAlert
relation toGovDist
model.Confirm that the
LocationAlert
model is correctly set up and that this relation is necessary for the functionality of the application.
1408-1430
: Introduced a new modelLocationAlert
with fieldsid
,active
,text
,level
,country
, andgovDist
.This model is crucial for the new feature of dynamic location-based alerts. Ensure that all fields are correctly used in the application logic and that the relations (
country
andgovDist
) are properly indexed for performance.
1426-1430
: Defined new enumLocationAlertLevel
with valuesINFO
,WARN
, andCRITICAL
.This enum is essential for categorizing the severity of location alerts. Ensure that the application logic correctly handles these levels.
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.
Actionable comments posted: 0
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.
Actionable comments posted: 0
Quality Gate passedIssues Measures |
Pull Request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: IN-941
What is the new behavior?
Does this introduce a breaking change?
Other information
Screen.Recording.2024-03-22.at.1.45.15.PM.mov
Summary by CodeRabbit
New Features
Enhancements
lightPink
andlightGreen
) to improve visual elements.Bug Fixes
Documentation