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.

Sign up for GitHub

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

Jump to bottom

Seasonal series completion icon #4188

Merged
merged 14 commits into from
Nov 25, 2022
Merged

Seasonal series completion icon #4188

merged 14 commits into from
Nov 25, 2022

Conversation

tibvdm
Copy link
Collaborator

@tibvdm tibvdm commented Nov 23, 2022 •
edited
Loading

Temporal overlay for Christmas

assembled_plain

Ho Ho Ho, Santa has found his way to Dodona. He will be sitting on your status icons in order to watch your progress. Santa left his hat on every single status icon. When you hover the icon, he will come forward and take a peek.

Screenshot 2022-11-23 at 11 56 34

Screenshot 2022-11-23 at 11 56 55

This PR adds:

  • A new SeasonalHelper that selects some css-styling based on the current time. At this moment I only do a check for Christmas (Dec 7 until Dec 31)
  • Christmas stylesheet in orde to place a santa hat on top of "green checkmarks". On hover this will become Santa. Santa is also present on "rainbow checkmarks" without the hover functionality.

Screenshot 2022-11-25 at 09 44 19

@tibvdm tibvdm requested review from jorg-vr and a team as code owners November 23, 2022 11:03
@tibvdm tibvdm requested review from niknetniko and removed request for a team November 23, 2022 11:03
@bmesuere bmesuere changed the title Temporal icon Seasonal series completion icon Nov 23, 2022
@jorg-vr jorg-vr added the enhancement A change that isn't substantial enough to be called a feature label Nov 23, 2022
jorg-vr
jorg-vr requested changes Nov 23, 2022
View reviewed changes
Copy link
Contributor

@jorg-vr jorg-vr left a comment

Choose a reason for hiding this comment

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

I really like the santa figure, thanks for the contribution!

BTW: I'll give you access to create branches on dodona, so you don't need to work from your own fork if you want to contribute more in the future.

app/helpers/temporal_helper.rb Outdated
@@ -0,0 +1,14 @@
module TemporalHelper
def christmas(current_time)
return current_time.month == 12 && current_time.day.between?(25, 31)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would include the 24th because Christmas eve also deserves a smiling Santa

Copy link
Member

Choose a reason for hiding this comment

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

I would even start this as soon as december first

Copy link
Contributor

Choose a reason for hiding this comment

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

My kids would strongly object doing this earlier than December 7th: no competition allowed!

app/views/series/_series_status.html.erb Outdated Show resolved Hide resolved
app/views/series/_series_status.html.erb Outdated Show resolved Hide resolved
app/assets/stylesheets/temporal/christmas.css.scss Outdated Show resolved Hide resolved
bmesuere
bmesuere requested changes Nov 23, 2022
View reviewed changes
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

Can you run your SVGs through https://www.svgminify.com/ or something similar?

I would also rename all "temporal" things to "seasonal".

Gemfile.lock Outdated
@@ -227,7 +227,10 @@ GEM
addressable (~> 2.7)
Copy link
Member

Choose a reason for hiding this comment

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

Can you exclude this file from the PR?

jorg-vr
jorg-vr requested changes Nov 24, 2022
View reviewed changes
Copy link
Contributor

@jorg-vr jorg-vr left a comment

Choose a reason for hiding this comment

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

Except for the lockfile, everything looks good.

Side note: Once you have applied changes you can click on the circle arrows (Top right) to re request a review, which will notify me

Gemfile.lock Outdated
@@ -1,579 +0,0 @@
GEM
Copy link
Contributor

Choose a reason for hiding this comment

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

You chose the wrong method to exclude it...
Please do not remove it from the repo, only exclude changes made by you

@bmesuere bmesuere added the deploy mestra Request a deployment on mestra label Nov 24, 2022
@bmesuere bmesuere added deploy mestra Request a deployment on mestra and removed deploy mestra Request a deployment on mestra labels Nov 24, 2022
bmesuere
bmesuere requested changes Nov 24, 2022
View reviewed changes
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

I deployed this locally and it looks really nice!

I do agree with @chvp that this might be a bit much if we apply it to all series icons for the entire month and add other seasonal decorations in the future.

We could reduce the number of appearances by only applying the hat to the "green checkmark" icon (showing santa on hover) and show santa (without hover) instead of the "rainbow checkmark". This would make santa some kind of achievement. What do the others think?

I also checked if we could replace the icons that are shown on correct submission by a set of christmas-themed icons. Unfortunately, mdi doesn't include a lot of christmas icons.

Regarding the code: there are a lot of !important's in the css which is something we try to avoid. Are these strictly necessary?

@bmesuere bmesuere added feature New feature or request and removed deploy mestra Request a deployment on mestra enhancement A change that isn't substantial enough to be called a feature labels Nov 24, 2022
jorg-vr
jorg-vr approved these changes Nov 25, 2022
View reviewed changes
Copy link
Contributor

@jorg-vr jorg-vr left a comment

Choose a reason for hiding this comment

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

Could you update your PR description to match the final implementation?

chvp
chvp approved these changes Nov 25, 2022
View reviewed changes
app/views/series/_series_status.html.erb Outdated Show resolved Hide resolved
@tibvdm @chvp
Remove brackets from helper function call
de35cf9 
Co-authored-by: Charlotte Van Petegem <charlotte@vanpetegem.me>
@tibvdm
Copy link
Collaborator Author

tibvdm commented Nov 25, 2022

I updated the description and added an extra image to show the different overlay options.

@jorg-vr jorg-vr merged commit 1849d25 into dodona-edu:develop Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pdawyndt pdawyndt pdawyndt left review comments

@bmesuere bmesuere bmesuere requested changes

@chvp chvp chvp approved these changes

@jorg-vr jorg-vr jorg-vr approved these changes

@niknetniko niknetniko niknetniko approved these changes

Assignees

@tibvdm tibvdm

Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tibvdm @bmesuere @niknetniko @pdawyndt @jorg-vr @chvp