Skip to content

[FEAT] Dark theme + Other enhancements #572

Merged
BilelJegham merged 20 commits intoGeoGuess:masterfrom
Troplo:patch-1
Dec 17, 2022
Merged

[FEAT] Dark theme + Other enhancements #572
BilelJegham merged 20 commits intoGeoGuess:masterfrom
Troplo:patch-1

Conversation

@Troplo
Copy link
Copy Markdown
Collaborator

@Troplo Troplo commented Nov 19, 2022

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description

I've added a dark theme for the UI and the Google map using the native Vuetify dark mode property and Vuetify's CSS variable exposure, instead of statically setting hex codes in the CSS like background: #7fad94;, background: var(--v-footer-base); is used instead, and these properties can be adjusted in Vuetify's plugin config.

Additionally, I've fixed/changed a few things that have been bothering me throughout playing GeoGuess:

  • Pin (pinActive) for map now persists in LocalStorage.
  • Player name max length has been increased from 10 to 20.
  • Additional validation has been added in the frontend to check the player name length (previously it was as simple as changing the maxlength of the input which allowed infinite characters.)
  • VUE_APP_DEFAULT_THEME environment variable has been added to set the default theme for the instance (default is light if unset, valid options: dark, light)
  • Fixed enter key on room selection screen (was missing function parameter causing an error to be thrown), may fix [FEAT] Make it possible to use "ENTER" on keyboard to get to next page in Dialogs #124, I'm unsure if there's any other dialogs which should have enter to go to the next page.
  • Removing all characters from name selection will no longer set back to the default "Anonymous" text, as this was buggy and prevented people from easily setting a name of their choosing.
  • To prevent users from setting a blank name, the "Next" button will be disabled until all players have set a name.
  • Some fixes to grammar/spelling (English).

How Has This Been Tested?

The largest code changes would be the adoption of Vuetify's CSS variables in place of hexes.

I've deployed all my changes to my instance of GeoGuess and have been playing it for around a month with multiple players with most of the changes listed and have experienced no issues related to the changes.

Linting, and Jest passes for me, however Cypress sometimes fails with AssertionError: Timed out retrying after 4000ms: Expected to find element: "map", but never found it., I'm not sure if this was a problem before, a configuration mistake with GeoGuess itself locally, or if I changed something code wise to cause this (I couldn't find anything that would lead to this error, the map and game does function as expected in a web browser)

Screenshots (if appropriate):

Dark homepage
History page
Game page
Result page

@netlify
Copy link
Copy Markdown

netlify bot commented Nov 19, 2022 •
edited
Loading

👷 Deploy request for geoguess2 accepted.

Name Link
🔨 Latest commit 75cc7fa
🔍 Latest deploy log https://app.netlify.com/sites/geoguess2/deploys/6382d3eff7558600085f665e

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 20, 2022 •
edited
Loading

Codecov Report

Base: 51.47% // Head: 51.28% // Decreases project coverage by -0.19% ⚠️

Coverage data is based on head (e0ec5a8) compared to base (73d78f7).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #572      +/-   ##
==========================================
- Coverage   51.47%   51.28%   -0.20%     
==========================================
  Files          52       52              
  Lines         744      741       -3     
  Branches      206      205       -1     
==========================================
- Hits          383      380       -3     
  Misses        329      329              
  Partials       32       32
Impacted Files Coverage Δ
src/App.vue 100.00% <ø> (ø)
src/components/Maps.vue 100.00% <ø> (ø)
src/components/dialogroom/card/CardRoomMap.vue 100.00% <ø> (ø)
src/components/dialogroom/card/CardRoomName.vue 100.00% <ø> (ø)
.../components/dialogroom/card/CardRoomPlayerName.vue 100.00% <ø> (ø)
src/components/history/HistoryTable.vue 100.00% <ø> (ø)
...c/components/history/gameResult/HistoryMapArea.vue 100.00% <ø> (ø)
...omponents/history/gameResult/HistoryMapClassic.vue 100.00% <ø> (ø)
src/components/home/DialogCustomMap.vue 100.00% <ø> (ø)
src/components/home/MapsContainer.vue 100.00% <ø> (ø)
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@BilelJegham BilelJegham self-requested a review November 20, 2022 22:08
BilelJegham
BilelJegham requested changes Nov 26, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@BilelJegham BilelJegham left a comment

Choose a reason for hiding this comment

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

Hello 👋 ,

I really appreciate your contribution, it's been a while user ask for dark theme.

All tested with cypress are OK ✔️ on my side.

I make some suggestions and ask some questions.

I'm waiting for your feedback 😉

@Troplo
Resolve suggestions
932d1e6 
I've removed VUE_APP_DEFAULT_THEME env in place of checking the browser's current theme like suggested, and fixed other problems.
@Troplo
Fix spelling mistake
75cc7fa
@Troplo Troplo requested a review from BilelJegham November 27, 2022 03:46
@Troplo Troplo mentioned this pull request Dec 5, 2022
Merged
3 tasks
@Troplo Troplo changed the title Dark theme + Other enhancements [FEAT} Dark theme + Other enhancements Dec 5, 2022
@Troplo Troplo changed the title [FEAT} Dark theme + Other enhancements [FEAT] Dark theme + Other enhancements Dec 5, 2022
BilelJegham
BilelJegham approved these changes Dec 17, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@BilelJegham BilelJegham left a comment

Choose a reason for hiding this comment

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

It's all good for me.

Big thanks for this PR @Troplo

@BilelJegham BilelJegham enabled auto-merge (squash) December 17, 2022 15:11
@BilelJegham
Copy link
Copy Markdown
Member

BilelJegham commented Dec 17, 2022

@allcontributors please add @Troplo for code

@BilelJegham BilelJegham disabled auto-merge December 17, 2022 15:11
@allcontributors allcontributors bot mentioned this pull request Dec 17, 2022
Merged
@allcontributors
Copy link
Copy Markdown
Contributor

allcontributors bot commented Dec 17, 2022

@BilelJegham

I've put up a pull request to add @Troplo! 🎉

@BilelJegham BilelJegham enabled auto-merge (squash) December 17, 2022 15:12
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Dec 17, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
34.7% 34.7% Duplication

@BilelJegham BilelJegham merged commit 3635c4a into GeoGuess:master Dec 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BilelJegham BilelJegham BilelJegham approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEAT] Make it possible to use "ENTER" on keyboard to get to next page in Dialogs

2 participants

@Troplo @BilelJegham