Skip to content

appearance persistence and accessibility improvements#25

Merged
krekas merged 3 commits intoLaravelDaily:mainfrom
Zaingoya65:feature/appearance-persistence-and-ui-improvements
Feb 10, 2026
Merged

appearance persistence and accessibility improvements#25
krekas merged 3 commits intoLaravelDaily:mainfrom
Zaingoya65:feature/appearance-persistence-and-ui-improvements

Conversation

@Zaingoya65
Copy link
Copy Markdown
Contributor

  • Added theme_preference column to users table and updated User model.
  • Implemented quick-access theme toggle in the header.
  • Added "Settings" link to the sidebar for better discoverability.
  • Fixed layout nesting issues in header and sidebar.
  • Includes comprehensive feature tests (AppearanceUpdateTest.php).

@krekas
Copy link
Copy Markdown
Collaborator

krekas commented Feb 9, 2026

Hi, everything looks good but we need you to make some adjustment:

  1. There's no need for the notification after the appearance is changed, especially when the message is appearance-updated
image
  1. The added dropdown doesn't have active state
image
  1. I don't think adding settings link to the navigation is necessary. Usually it's under the user, let's remove it.
image

@Zaingoya65
Copy link
Copy Markdown
Contributor Author

I have updated the PR according to your feedback, I removed the notification, added active states to the theme toggle, and removed the settings link from the sidebar.

Ready for another look!

@krekas
Copy link
Copy Markdown
Collaborator

krekas commented Feb 10, 2026

Didn't see at first. Remove the composer.lock from the commit

@Zaingoya65
Copy link
Copy Markdown
Contributor Author

Thanks for catching that! I have removed composer.lock from the PR. It was accidentally included in one of my previous commits.
Everything is now updated according to your feedback

@krekas krekas merged commit ebc6c5d into LaravelDaily:main Feb 10, 2026
@PovilasKorop
Copy link
Copy Markdown
Collaborator

@Zaingoya65 thank you for your PR and the fixes, I really appreciate it.

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.

3 participants