-
Notifications
You must be signed in to change notification settings - Fork 13
chore: refactored banner. Failed at admin panel implementation #574
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import { MarqueeWrapper } from '../MarqueeWrapper' | ||
|
||
interface GenericBannerProps { | ||
message: string | ||
backgroundColor?: string | ||
icon?: string | ||
} | ||
|
||
export function GenericBanner({ message, backgroundColor = 'bg-purple-1', icon }: GenericBannerProps) { | ||
return ( | ||
<MarqueeWrapper backgroundColor={backgroundColor} direction="left"> | ||
<span className="z-10 mx-4 text-sm font-semibold"> | ||
{icon && <span className="mr-2">{icon}</span>} | ||
{message} | ||
</span> | ||
</MarqueeWrapper> | ||
) | ||
} | ||
Comment on lines
+9
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance component configurability and accessibility The component needs several improvements:
-export function GenericBanner({ message, backgroundColor = 'bg-purple-1', icon }: GenericBannerProps) {
+export function GenericBanner({ message, backgroundColor = theme.banner.default, icon }: GenericBannerProps) {
return (
<MarqueeWrapper backgroundColor={backgroundColor} direction="left">
<span className="z-10 mx-4 text-sm font-semibold">
- {icon && <span className="mr-2">{icon}</span>}
+ {icon && (
+ <span
+ className="mr-2"
+ role="img"
+ aria-label={icon.ariaLabel}
+ >
+ {icon.symbol}
+ </span>
+ )}
{message}
</span>
</MarqueeWrapper>
)
} Would you like me to help create test cases for this component?
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,16 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { GenericBanner } from './GenericBanner' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import config from '@/config/routesUnderMaintenance' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
interface MaintenanceBannerProps { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pathname: string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export function MaintenanceBanner({ pathname }: MaintenanceBannerProps) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const isUnderMaintenance = config.routes.some((route) => pathname.startsWith(route)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!isUnderMaintenance) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return null | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return <GenericBanner message="Under maintenance" icon="⚠️" /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thought: we should use our icons, but I believe that is changing from current version to pw There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+1
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add error boundary for route configuration issues The component should gracefully handle potential configuration errors. +import { ErrorBoundary } from '@/components/ErrorBoundary'
+
export function MaintenanceBanner({ pathname }: MaintenanceBannerProps) {
const isUnderMaintenance = config.routes.some((route) => pathname.startsWith(route))
if (!isUnderMaintenance) {
return null
}
- return <GenericBanner message="Under maintenance" icon="⚠️" />
+ return (
+ <ErrorBoundary fallback={null}>
+ <GenericBanner message="Under maintenance" icon="⚠️" />
+ </ErrorBoundary>
+ )
} 📝 Committable suggestion
Suggested change
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,23 @@ | ||
import { usePathname } from 'next/navigation' | ||
import { MarqueeWrapper } from '../MarqueeWrapper' | ||
import { GenericBanner } from './GenericBanner' | ||
import { MaintenanceBanner } from './MaintenanceBanner' | ||
import { MAINTAINABLE_ROUTES } from '@/config/routesUnderMaintenance' | ||
|
||
export function Banner() { | ||
const pathname = usePathname() | ||
const isCashout = pathname.startsWith('/cashout') | ||
const isRequest = pathname.startsWith('/request') | ||
if (!pathname) return null | ||
|
||
// Return null if not on cashout or request pages | ||
if (!isCashout && !isRequest) return null | ||
// First check for maintenance | ||
const maintenanceBanner = <MaintenanceBanner pathname={pathname} /> | ||
if (maintenanceBanner) return maintenanceBanner | ||
|
||
// Show maintenance message for cashout | ||
if (isCashout) { | ||
return ( | ||
<MarqueeWrapper backgroundColor="bg-purple-1" direction="left"> | ||
<span className="z-10 mx-4 text-sm font-semibold">⚠️ Under maintenance</span> | ||
</MarqueeWrapper> | ||
) | ||
// Show beta message for all request paths (create and pay) unless under maintenance | ||
if (pathname.startsWith(MAINTAINABLE_ROUTES.REQUEST)) { | ||
return <GenericBanner message="Beta feature - share your thoughts!" backgroundColor="bg-purple-1" /> | ||
} | ||
|
||
// Show beta message for request | ||
return ( | ||
<MarqueeWrapper backgroundColor="bg-purple-1" direction="left"> | ||
<span className="z-10 mx-4 text-sm font-semibold">Beta feature - share your thoughts!</span> | ||
</MarqueeWrapper> | ||
) | ||
return null | ||
} | ||
|
||
export { GenericBanner } from './GenericBanner' | ||
export { MaintenanceBanner } from './MaintenanceBanner' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
interface MaintenanceConfig { | ||
routes: string[] | ||
} | ||
|
||
export const MAINTAINABLE_ROUTES = { | ||
CASHOUT: '/cashout', | ||
REQUEST: '/request', | ||
SEND: '/send', | ||
CLAIM: '/claim', | ||
} as const | ||
|
||
// Static configuration - edit this file to change maintenance state | ||
const config: MaintenanceConfig = { | ||
routes: [ | ||
MAINTAINABLE_ROUTES.CASHOUT, // Routes under maintenance | ||
], | ||
} | ||
|
||
export default config |
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.
🛠️ Refactor suggestion
Consider adding aria labels for accessibility
The icon prop should include accessibility attributes for screen readers.
📝 Committable suggestion