view shelter 2 (DEV-1079) #781
Conversation
Reviewer's Guide by SourceryThis PR implements the shelter view page UI, introducing new reusable React components and enum display mappings. The implementation includes multiple sections for displaying shelter information such as general info, entry requirements, special restrictions, and room styles. The changes also include styling updates and new color variables. Class diagram for new React componentsclassDiagram
class Button {
+size: 'sm' | 'md' | 'lg'
+variant: 'primary' | 'secondary'
+Button(props: IButtonProps)
}
class CardWrapper {
+children: ReactNode
+px: 'px-0' | 'px-6'
+pb: 'pb-0' | 'pb-6'
+title: string
+CardWrapper(props: ICardWrapperProps)
}
class Pill {
+type: 'success'
+label: string
+Pill(props: IPillProps)
}
class PillContainer {
+data: string[]
+type: 'success'
+maxVisible: number
+PillContainer(props: IPillContainerProps)
}
Button --> CardWrapper
PillContainer --> Pill
Class diagram for Shelter Page componentsclassDiagram
class Header {
+Header(shelter: ViewShelterQuery['shelter'])
}
class OperationHours {
+OperationHours()
}
class Actions {
+Actions()
}
class EntryRequirements {
+EntryRequirements(shelter: ViewShelterQuery['shelter'])
}
class GeneralInfo {
+GeneralInfo(shelter: ViewShelterQuery['shelter'])
}
class GeneralServices {
+GeneralServices(shelter: ViewShelterQuery['shelter'])
}
class SpecialRestrictions {
+SpecialRestrictions(shelter: ViewShelterQuery['shelter'])
}
class ShelterTypes {
+ShelterTypes(shelter: ViewShelterQuery['shelter'])
}
class RoomStyles {
+RoomStyles(shelter: ViewShelterQuery['shelter'])
}
GeneralInfo --> GeneralServices
ShelterPage --> Header
ShelterPage --> OperationHours
ShelterPage --> Actions
ShelterPage --> EntryRequirements
ShelterPage --> GeneralInfo
ShelterPage --> SpecialRestrictions
ShelterPage --> ShelterTypes
ShelterPage --> RoomStyles
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Davit-BetterAngels - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Avoid using dangerouslySetInnerHTML with string concatenation as it poses security risks (link)
Overall Comments:
- There's a typo in the 'Special Situatioin Restrictions' title that should be fixed
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| {shelter?.entryInfo && ( | ||
| <div | ||
| className="flex gap-1" | ||
| dangerouslySetInnerHTML={{ |
There was a problem hiding this comment.
🚨 issue (security): Avoid using dangerouslySetInnerHTML with string concatenation as it poses security risks
Consider using regular JSX with template literals instead, like:
| }) { | ||
| if (!shelter?.specialSituationRestrictions?.length) return null; | ||
| return ( | ||
| <CardWrapper title="Special Situatioin Restictions"> |
There was a problem hiding this comment.
nitpick (typo): Fix typo in CardWrapper title
| <CardWrapper title="Special Situatioin Restictions"> | |
| <CardWrapper title="Special Situation Restrictions"> |
| <div className="pb-6"> | ||
| <PillContainer | ||
| maxVisible={5} | ||
| data={ |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the data transformation using filter and map instead of reduce for improved readability.
The data transformation can be simplified while maintaining all safety checks. Using filter() and map() makes the intent clearer than reduce() here since we're transforming data rather than accumulating it.
data={
shelter?.generalServices
?.filter(service => service.name && enumDisplayGeneralServiceChoices[service.name])
.map(service => enumDisplayGeneralServiceChoices[service.name]) ?? []
}This maintains all null checks while being more direct about the data flow: first filter out invalid services, then transform the remaining ones.
| <CardWrapper title="Entry Requirements"> | ||
| <div className="flex flex-col gap-2"> | ||
| {shelter?.entryRequirements.map((requirement, idx) => { | ||
| if (!requirement.name) return null; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!requirement.name) return null; | |
| if (!requirement.name) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| if (service.name) { | ||
| const displayName = | ||
| enumDisplayGeneralServiceChoices[service.name]; | ||
| if (displayName) acc.push(displayName); |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (displayName) acc.push(displayName); | |
| if (displayName) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| }: { | ||
| shelter?: ViewShelterQuery['shelter']; | ||
| }) { | ||
| if (!shelter?.roomStyles?.length) return null; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!shelter?.roomStyles?.length) return null; | |
| if (!shelter?.roomStyles?.length) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| }: { | ||
| shelter?: ViewShelterQuery['shelter']; | ||
| }) { | ||
| if (!shelter?.shelterTypes?.length) return null; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!shelter?.shelterTypes?.length) return null; | |
| if (!shelter?.shelterTypes?.length) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| }: { | ||
| shelter?: ViewShelterQuery['shelter']; | ||
| }) { | ||
| if (!shelter?.specialSituationRestrictions?.length) return null; |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!shelter?.specialSituationRestrictions?.length) return null; | |
| if (!shelter?.specialSituationRestrictions?.length) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| const baseClasses = 'rounded-[20px] inline-flex items-center justify-center'; | ||
|
|
||
| const typeClasses = { | ||
| success: 'bg-success-90 text-primary-20 px-4 py-1', |
There was a problem hiding this comment.
the color success-90 is only defined in the context of shelter-web. that's ok for today, but we may need add global tailwind config file in libs somwhere. for now maybe just leave a TODO comment?
also, should px-4 py-1 go with baseClasses?
There was a problem hiding this comment.
paddings no because if border comes the size will be different
There was a problem hiding this comment.
aaah ok.
if a different variant can have a border, then another way is to include border in baseClasses (with same color as background), and then just override the border-color. but not necessary
| success: 'bg-success-90 text-primary-20 px-4 py-1', | ||
| }; | ||
|
|
||
| const pillClass = clsx(baseClasses, typeClasses[type], className); |
There was a problem hiding this comment.
just FYI: twMerge together with clsx (like in mergeCss) can be really useful here
to allow className to override everyting:
const pillClass = mergeCss(baseClasses, typeClasses[type], className);
to allow adding className, but never overriding baseClasses or typeClasses[type]:
const pillClass = mergeCss(className, baseClasses, typeClasses[type]);
Davit-BetterAngels
temporarily deployed
to
preview
GitHub Actions
Inactive
|
🚀 Expo continuous deployment is ready for betterangels!
iOS Simulator Build: Simulator Build Link |
Davit-BetterAngels
temporarily deployed
to
preview
GitHub Actions
Inactive
DEV-1079
Summary by Sourcery
Enhance the shelter page by introducing new components for modularity and readability, including Button, Pill, CardWrapper, and various shelter-specific components. Update enum display mappings and global styles to support these changes. Specify Yarn as the package manager in the project configuration.
New Features:
Enhancements:
Build:
Chores: