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

Add completion marker to daily challenge profile counter #11780

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+25 −3
Open

Add completion marker to daily challenge profile counter #11780

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9fc4d20
Add completion marker to daily challenge profile counter
bdach Jan 9, 2025
b8adbac
Adjust radius of inner box
bdach Jan 9, 2025
5f2ab58
Remove superfluous div
bdach Jan 9, 2025
e69ab6a
Apply review suggestions
bdach Jan 15, 2025
18b2ecf
Merge branch 'master' into daily-challenge-completion-marker
bdach Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion resources/css/bem/daily-challenge.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,27 @@
background: hsl(var(--hsl-b4));
border-radius: @border-radius-large;
min-width: 0;
position: relative;
display: flex;
align-items: center;
padding: 3px;

&--played-today {
border: 2px solid @osu-colour-lime-1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is 1px on the design file but I find that 1px looks terrible on the rounded corners:

1px 2px
1736414918 1736414351

Arguably even 2px looks not great. Maybe 3px even could be considered. cc @ppy/team-design

Copy link
Collaborator

Choose a reason for hiding this comment

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

the extra spacing between the day count box and the green border seems a bit weird?

also dunno if matters but the popup box covers the check mark a little bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the extra spacing between the day count box and the green border seems a bit weird?

I mean yeah maybe. I'm just following design direction here 🤷

also dunno if matters but the popup box covers the check mark a little bit

Yeah it does and was also unsure if it mattered. I guess maybe the design peoples can say?

Just for visual reference here's a screenshot:

1736416429

Copy link
Collaborator

Choose a reason for hiding this comment

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

this should only change the border-color in the modifier here with the rest border set in the base class (with transparent color) to have consistent positioning between played and not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the process of applying this I noticed that the border even if transparent was also stacking onto the existing 3px of padding (see screenshots below, with the border green colour changed to transparent to show the difference):

before after
1736948658 1736948639

which I didn't want to do and I don't think the design was going for, so I reduced the padding to 1px to compensate.


&::before {
.fas();
background-color: @osu-colour-b6;
border-radius: 50%;
color: @osu-colour-lime-1;
content: @fa-var-check-circle;
font-size: 16px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

there has been an attempt on marking icon-specific hard coded font-size - instead of using variable - with // icon size comment

position: absolute;
right: -8px;
top: -8px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

-0.5em works here (or 0 with ±50% translate transform if feeling fancy)

}
}

&__name {
font-size: @font-size--normal;
padding: 0 5px;
Expand All @@ -20,7 +37,7 @@
}

&__value-box {
border-radius: @border-radius-large;
border-radius: @border-radius-small;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted as per mention in #11597:

also, the corner radius of that darker block is supposed to be 3px, it's 6px on osu-web rn for some reason.

background: hsl(var(--hsl-b6));
padding: 5px 10px;
}
Expand Down
5 changes: 4 additions & 1 deletion resources/js/profile-page/daily-challenge.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import DailyChallengeUserStatsJson from 'interfaces/daily-challenge-user-stats-json';
import { autorun } from 'mobx';
import { observer } from 'mobx-react';
import * as moment from 'moment';
import * as React from 'react';
import { renderToStaticMarkup } from 'react-dom/server';
import { classWithModifiers, Modifiers } from 'utils/css';
Expand Down Expand Up @@ -122,10 +123,12 @@ export default class DailyChallenge extends React.Component<Props> {
return null;
}

const playedToday = this.props.stats.last_update != null && moment.utc(this.props.stats.last_update).isSame(Date.now(), 'day');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is gonna do timezones correctly:

https://momentjs.com/docs/#/query/is-same/

If the two moments have different timezones, the timezone of the first moment will be used for the comparison.

and it looked correct when I messed around with my timezone...

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah that seems correct (until someone complains it's not)

alternatively new Date(...).toISOString() spits out iso8601 string which has utc timezone and can then be sliced appropriately (not sure if better)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and can then be sliced appropriately (not sure if better)

I uh... am not confident in my ability to do so, so I'd rather not if that's fine with you 😅


return (
<div
ref={this.valueRef}
className='daily-challenge'
className={classWithModifiers('daily-challenge', { 'played-today': playedToday })}
onMouseOver={this.onMouseOver}
>
<div className='daily-challenge__name'>
Expand Down
Loading