Skip to content
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

feat: improve project submisions dark mode #4324

Merged

Conversation

scheals
Copy link
Contributor

@scheals scheals commented Jan 8, 2024

Because

When visiting a full project submissions page the UX of dark mode was worse of light mode potentially due to some rules not applying.

This PR

  • Gives the active page the lovely Odin gold colour just like light mode does.
  • Attempts to harmonize link colour with the Back to lesson link at the top of the page.
  • Makes disabled Prev / Next more visually distinct and intuitively disabled depending on the circumstances.

Issue

Closes #4307

Additional Information

This is directly pertaining to the PR, rest is about the process more than anything else:
I duplicated the rules like text-gold-500 because of specificity, otherwise it wouldn't work. Looks weird, feels weird but works just fine! Not sure if those &:active rules are even needed - it is only seen for a split second on slow network after clicking a link, right?

According to the Contributing Guide you're supposed to run rubocop and not bundle exec rubocop - same with rspec. Is this intentional? I have offences with rubocop, all to do with Capybara but when I do bundle exec rubocop I don't see any issues. Neither the main rubocop.yml nor the one in spec/ have any rules that seem to affect Capybara. It seems like my global rubocop-capybara is 2.20.0 and the one in the Gemfile is 2.19.0 and this is the reason for mismatch. I have already submitted a PR to remove yarn test from the guide - it feels like every time a gem is used, it should be used with bundle exec. Perhaps this should also be the case with rails - every instance should be bin/rails instead.

Also, I have no idea if it is my setup or what, but there was plenty of bin/rails assets:clobber and bin/rails css:clobber for me before I could get webpack to actually reflect changes in the file on refresh. At some point I was thinking the changes should not be made in pagy.css!

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project Contributing Guide
  • The title of this PR follows the keyword: brief description of change format, using one of the following keywords:
    • Feature - adds new or amends existing user-facing behavior
    • Chore - changes that have no user-facing value, refactors, dependency bumps, etc
    • Fix - bug fixes
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • I have verified all tests and linters pass after making these changes.
  • If this PR addresses an open issue, it is linked in the Issue section
  • If applicable, this PR includes new or updated automated tests

@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4324 January 9, 2024 08:07 Inactive
@JoshDevHub
Copy link
Contributor

Going to leave the review of your changes to someone who knows more about the app's CSS structure(s).

But going to interject on the bit about the contributing document not running the linting and tests while prefixed with bundle exec. You're correct about all of this, but I think CONTRIBUTING.md can tell people to run bin/test instead (link to that script). This script will run the correct command for each tool (ie. using bundle exec), and it runs everything you need to pass CI out of a single command. So the developer can just do bin/test rather than entering 3 or 4 commands to check this stuff.

If you'd like to make that change, I would gladly approve it 😃

@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4324 January 10, 2024 08:26 Inactive
Copy link
Member

@KevinMulhern KevinMulhern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @scheals 💪

Also, I have no idea if it is my setup or what, but there was plenty of bin/rails assets:clobber and bin/rails css:clobber for me before I could get webpack to actually reflect changes in the file on refresh. At some point I was thinking the changes should not be made in pagy.css!

This has happened to me a few times as well. We fairly recently switched to CSS bundling and it seems to have started since then. It's likely just something minor we've misconfigured. Would you mind make an issue for it?

@KevinMulhern KevinMulhern merged commit e5be408 into TheOdinProject:main Jan 10, 2024
3 checks passed
@scheals scheals deleted the feature/better-darkmode-submissions branch January 10, 2024 09:45
@scheals
Copy link
Contributor Author

scheals commented Jan 10, 2024

@KevinMulhern #4327 As you requested :D

@scheals
Copy link
Contributor Author

scheals commented Jan 10, 2024

@JoshDevHub I investigated this and it turns out there should be more work done with the docs #4328

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Feature Request: Improve dark mode submission page selection readibility
3 participants