feat: restyle navbar #284

Merged

ptran merged 6 commits from homepage into main 2025-06-04 10:23:17 +00:00

Conversation 7 Commits 6 Files changed 9 +268 -132

ptran commented 2025-06-04 09:51:30 +00:00

Member

Copy link

 

image.png

72 KiB

image.png

20 KiB

image.png

27 KiB

image.png

36 KiB

ptran added 3 commits 2025-06-04 09:51:30 +00:00

feat: adjust game grid ada291b640

feat: improve game grid e5e47ff96e

feat: restyle navbar 64923719e4

ptran force-pushed homepage from 64923719e4 to ccfc74891c 2025-06-04 09:51:39 +00:00 Compare

forgejo-actions commented 2025-06-04 09:52:07 +00:00

Copy link

👮‍♀️⚠️ This is a friendly reminder that the diff size of this PR is bigger than 200 lines we aim for. Please consider splitting this PR into more digestible pieces!

👮‍♀️⚠️ This is a friendly reminder that the diff size of this PR is bigger than 200 lines we aim for. Please consider splitting this PR into more digestible pieces!

forgejo-actions added the

size

large

frontend

labels 2025-06-04 09:52:07 +00:00

ptran force-pushed homepage from ccfc74891c to 463090b224 2025-06-04 09:52:55 +00:00 Compare

Claude commented 2025-06-04 09:53:16 +00:00

Collaborator

Copy link

Code Review Results

Overall Assessment: The navbar restyle looks visually appealing with improved mobile responsiveness and better UX. However, there are several technical issues that need attention.

Issues Found:

1. Missing imports in navbar.component.ts (line 13-17): is imported but not used. Please remove unused import.

2. Inconsistent styling in navbar.component.html (line 21): Mix of Tailwind utilities and custom classes. Consider standardizing on one approach for consistency.

3. Missing error handling in home.component.ts (line 1): Important error handling imports were removed without replacement. Ensure error handling is still functional.

4. Accessibility issues in navbar.component.html (lines 32-94): New buttons lack proper ARIA labels and keyboard navigation support. Please add accessibility attributes.

5. Code duplication in navbar.component.html (lines 61-82): Deposit/transaction modal functionality is now duplicated between navbar and home component. Consider creating a shared service.

6. Hardcoded text in navbar.component.html (lines 30-175): German text is hardcoded without i18n support. Consider adding internationalization for future scalability.

Recommendations:

• Remove unused imports
• Add proper ARIA labels for accessibility
• Ensure error handling still works after removing imports from home component
• Consider extracting modal management to a shared service

The visual improvements are great, but please address these technical concerns before merging.

## Code Review Results **Overall Assessment:** The navbar restyle looks visually appealing with improved mobile responsiveness and better UX. However, there are several technical issues that need attention. **Issues Found:** 1. **Missing imports in navbar.component.ts (line 13-17):** is imported but not used. Please remove unused import. 2. **Inconsistent styling in navbar.component.html (line 21):** Mix of Tailwind utilities and custom classes. Consider standardizing on one approach for consistency. 3. **Missing error handling in home.component.ts (line 1):** Important error handling imports were removed without replacement. Ensure error handling is still functional. 4. **Accessibility issues in navbar.component.html (lines 32-94):** New buttons lack proper ARIA labels and keyboard navigation support. Please add accessibility attributes. 5. **Code duplication in navbar.component.html (lines 61-82):** Deposit/transaction modal functionality is now duplicated between navbar and home component. Consider creating a shared service. 6. **Hardcoded text in navbar.component.html (lines 30-175):** German text is hardcoded without i18n support. Consider adding internationalization for future scalability. **Recommendations:** - Remove unused imports - Add proper ARIA labels for accessibility - Ensure error handling still works after removing imports from home component - Consider extracting modal management to a shared service The visual improvements are great, but please address these technical concerns before merging.

Claude commented 2025-06-04 09:53:29 +00:00

Collaborator

Copy link

Code Review - Minor Issues Found

The navbar restyle looks great overall! However, I found a few minor issues that should be addressed:

frontend/src/app/feature/home/home.component.ts:

• Line 5: Unused import - should be removed

frontend/src/app/shared/components/navbar/navbar.component.ts:

• Lines 6, 13: Inconsistent import formatting - some imports use spaces around braces while others don't. Please follow consistent formatting per project style guide

Overall Assessment:
✅ Good visual improvements to the navbar
✅ Proper mobile responsive design
✅ Nice use of SVG icons and modern styling
⚠️ Minor cleanup needed for unused imports and formatting consistency

The functionality appears to be properly moved from home to navbar component. Just need the small cleanup items addressed.

## Code Review - Minor Issues Found The navbar restyle looks great overall\! However, I found a few minor issues that should be addressed: **frontend/src/app/feature/home/home.component.ts:** - **Line 5**: Unused import - should be removed **frontend/src/app/shared/components/navbar/navbar.component.ts:** - **Lines 6, 13**: Inconsistent import formatting - some imports use spaces around braces while others don't. Please follow consistent formatting per project style guide **Overall Assessment:** ✅ Good visual improvements to the navbar ✅ Proper mobile responsive design ✅ Nice use of SVG icons and modern styling ⚠️ Minor cleanup needed for unused imports and formatting consistency The functionality appears to be properly moved from home to navbar component. Just need the small cleanup items addressed.

ptran force-pushed homepage from 463090b224 to 1e77beb7b6 2025-06-04 09:53:54 +00:00 Compare

Claude commented 2025-06-04 09:55:08 +00:00

Collaborator

Copy link

Code Review for Navbar Restyle

Overall, this is a solid UI improvement that modernizes the navbar with better visual hierarchy and moves essential functionality to a more accessible location. However, there are several issues that need to be addressed:

✅ Positive Changes:

• Improved navbar styling with better visual hierarchy and modern design
• Added SVG icons for better UX
• Moved deposit and transaction functionality to navbar for easier access
• Better responsive design with improved mobile menu

⚠️ Issues to Address:

1. Code Duplication (Critical)

• Files: lines 55-72 and previous
• The deposit and transaction modal logic is now duplicated between navbar and home components. This violates DRY principles and creates maintenance issues.

2. Missing Import Cleanup (Minor)

• File: line 5
• Unused import should be removed.

3. Incomplete Functionality Removal (Major)

• File: lines 65-66
• Methods and related functionality are still present but orphaned. The confirmation modal logic appears incomplete.

4. Potential Layout Issues (Minor)

• File: lines 23-60
• The new grid layout (3 games top row, 2 games bottom row) might look unbalanced. Consider adding responsive breakpoints for better visual balance.

5. Accessibility Concerns (Minor)

• File: lines 25-120
• SVG icons lack aria-labels for screen readers.

🔧 Recommendations:

1. Extract modal management logic into a shared service to eliminate duplication
2. Remove unused imports and orphaned methods
3. Add proper aria-labels to SVG icons
4. Test the new game grid layout on various screen sizes
5. Consider adding loading states for the new navbar buttons

The changes improve the overall UX significantly, but the code quality issues should be addressed before merging.

## Code Review for Navbar Restyle Overall, this is a solid UI improvement that modernizes the navbar with better visual hierarchy and moves essential functionality to a more accessible location. However, there are several issues that need to be addressed: ### ✅ Positive Changes: - Improved navbar styling with better visual hierarchy and modern design - Added SVG icons for better UX - Moved deposit and transaction functionality to navbar for easier access - Better responsive design with improved mobile menu ### ⚠️ Issues to Address: **1. Code Duplication (Critical)** - **Files**: lines 55-72 and previous - The deposit and transaction modal logic is now duplicated between navbar and home components. This violates DRY principles and creates maintenance issues. **2. Missing Import Cleanup (Minor)** - **File**: line 5 - Unused import should be removed. **3. Incomplete Functionality Removal (Major)** - **File**: lines 65-66 - Methods and related functionality are still present but orphaned. The confirmation modal logic appears incomplete. **4. Potential Layout Issues (Minor)** - **File**: lines 23-60 - The new grid layout (3 games top row, 2 games bottom row) might look unbalanced. Consider adding responsive breakpoints for better visual balance. **5. Accessibility Concerns (Minor)** - **File**: lines 25-120 - SVG icons lack aria-labels for screen readers. ### 🔧 Recommendations: 1. Extract modal management logic into a shared service to eliminate duplication 2. Remove unused imports and orphaned methods 3. Add proper aria-labels to SVG icons 4. Test the new game grid layout on various screen sizes 5. Consider adding loading states for the new navbar buttons The changes improve the overall UX significantly, but the code quality issues should be addressed before merging.

Claude commented 2025-06-04 09:55:43 +00:00

Collaborator

Copy link

Code Review for PR #284 - feat: restyle navbar

I've reviewed the changes and overall the navbar restyling looks good with improved visual design and UX. However, I've identified a few issues that need attention:

Issues Found:

frontend/src/app/feature/home/home.component.ts (lines 5, 64)

• Unused import: on line 5 should be removed
• Unused property: on line 64 should be removed since it's no longer referenced

frontend/src/app/feature/home/home.component.ts (line 42)

• Unused property: and its related method appear to be leftover code that should be cleaned up if not used

frontend/src/app/shared/components/navbar/navbar.component.html (line 155)

• Hard-coded currency symbol: There's a symbol on line 155 in the mobile menu that doesn't match the component's pattern of using Angular's CurrencyPipe elsewhere

Positive Changes:

• Great visual improvements with consistent styling and better spacing
• Good addition of SVG icons for better UX
• Proper responsive design for mobile and desktop
• Clean component organization by moving deposit/transaction functionality to navbar

Minor Suggestions:

• Consider extracting the repetitive SVG icons into a shared icon component for better maintainability
• The game grid layout change from 3 columns to 4 columns looks good but ensure it works well on different screen sizes

The functionality improvements and visual enhancements are solid, just needs the cleanup of unused code.

## Code Review for PR #284 - feat: restyle navbar I've reviewed the changes and overall the navbar restyling looks good with improved visual design and UX. However, I've identified a few issues that need attention: ### Issues Found: **frontend/src/app/feature/home/home.component.ts (lines 5, 64)** - Unused import: on line 5 should be removed - Unused property: on line 64 should be removed since it's no longer referenced **frontend/src/app/feature/home/home.component.ts (line 42)** - Unused property: and its related method appear to be leftover code that should be cleaned up if not used **frontend/src/app/shared/components/navbar/navbar.component.html (line 155)** - Hard-coded currency symbol: There's a symbol on line 155 in the mobile menu that doesn't match the component's pattern of using Angular's CurrencyPipe elsewhere ### Positive Changes: - Great visual improvements with consistent styling and better spacing - Good addition of SVG icons for better UX - Proper responsive design for mobile and desktop - Clean component organization by moving deposit/transaction functionality to navbar ### Minor Suggestions: - Consider extracting the repetitive SVG icons into a shared icon component for better maintainability - The game grid layout change from 3 columns to 4 columns looks good but ensure it works well on different screen sizes The functionality improvements and visual enhancements are solid, just needs the cleanup of unused code.

jleibl requested changes 2025-06-04 09:56:28 +00:00

Dismissed

jleibl left a comment

Member

fix button colors, stylings. please also make sure all icons match

fix button colors, stylings. please also make sure all icons match

ptran added 1 commit 2025-06-04 10:15:08 +00:00

feat: adjust icons 4331484d48

ptran added 1 commit 2025-06-04 10:15:28 +00:00

style: fix pipelines 23710a0553

jank approved these changes 2025-06-04 10:22:52 +00:00

jank dismissed jleibl's review 2025-06-04 10:23:02 +00:00

Reason:

nuh uh

ptran merged commit 43e321c0d6 into main 2025-06-04 10:23:17 +00:00

ptran deleted branch homepage 2025-06-04 10:23:17 +00:00

ptran referenced this pull request from a commit 2025-06-04 10:23:19 +00:00

Merge pull request 'feat: restyle navbar' (!284) from homepage into main

Commenting is not possible because the repository is archived.

Reviewers

No reviewers

jank

Labels

Clear labels   

Compat/Breaking

Breaking change that won't be backward compatible

  

Kind/Bug

Something is not working

  

Kind/Documentation

Documentation changes

  

Kind/Enhancement

Improve existing functionality

  

Kind/Feature

New functionality

  

Kind/Security

This is security issue

  

Kind/Testing

Issue or pull request related to testing

  

Priority

Critical

The priority is critical

  

Priority

High

The priority is high

  

Priority

Low

The priority is low

  

Priority

Medium

The priority is medium

  

Reviewed

Confirmed

Issue has been confirmed

  

Reviewed

Duplicate

This issue or pull request already exists

  

Reviewed

Invalid

Invalid issue

  

Reviewed

Won't Fix

This issue won't be fixed

  

Status

Abandoned

Somebody has started to work on this but abandoned work

  

Status

Blocked

Something is blocking this issue or pull request

  

Status

Need More Info

Feedback is required to reproduce issue or to continue work

  

backend

  

ci

  

docs

  

frontend

  

lgtm

blocked

  

lgtm

done

  

lgtm

need 1

  

size

huge

  

size

large

  

size

medium

  

size

small

No labels

Compat/Breaking

Kind/Bug

Kind/Documentation

Kind/Enhancement

Kind/Feature

Kind/Security

Kind/Testing

Priority

Critical

Priority

High

Priority

Low

Priority

Medium

Reviewed

Confirmed

Reviewed

Duplicate

Reviewed

Invalid

Reviewed

Won't Fix

Status

Abandoned

Status

Blocked

Status

Need More Info

backend

ci

docs

frontend

lgtm

blocked

lgtm

done

lgtm

need 1

size

huge

size

large

size

medium

size

small

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

5 participants

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: SZUT/casino#284

No description provided.