Add completion marker to daily challenge profile counter #11780
Conversation
| position: relative; | ||
|
|
||
| &--played-today { | ||
| border: 2px solid @osu-colour-lime-1; |
There was a problem hiding this comment.
This is 1px on the design file but I find that 1px looks terrible on the rounded corners:
| 1px | 2px |
|---|---|
 |
 |
Arguably even 2px looks not great. Maybe 3px even could be considered. cc @ppy/team-design
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
| @@ -20,7 +40,7 @@ | |||
| } | |||
|
|
|||
| &__value-box { | |||
| border-radius: @border-radius-large; | |||
| border-radius: @border-radius-small; | |||
There was a problem hiding this comment.
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.
| @@ -122,26 +123,30 @@ 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'); | |||
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 😅
| 'users.show.daily_challenge.unit.day', | ||
| { value: formatNumber(this.props.stats.playcount) }, | ||
| )} | ||
| <div className='daily-challenge__content'> |
There was a problem hiding this comment.
The reason for the existence of this div is that I need the ::before pseudo-element that is the check in the corner to be excluded from the flexbox, it takes space otherwise. If you have better ideas on how to implement this I'm open to changes.
There was a problem hiding this comment.
absolute-positioned element, which the element is, doesn't take space...?
There was a problem hiding this comment.
Huh. I guess I must have not tried some particular combination of css incantations because it does look to be not required now??? I love css.
(If you haven't figured out by now my css knowledge is somewhere at the level of "throw stuff against wall until it looks close enough" 😅)
| display: flex; | ||
| align-items: center; | ||
| padding: 3px; | ||
|
|
||
| &--played-today { | ||
| border: 2px solid @osu-colour-lime-1; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
|---|---|
 |
 |
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.
| border-radius: 50%; | ||
| color: @osu-colour-lime-1; | ||
| content: @fa-var-check-circle; | ||
| font-size: 16px; |
There was a problem hiding this comment.
there has been an attempt on marking icon-specific hard coded font-size - instead of using variable - with // icon size comment
| right: -8px; | ||
| top: -8px; |
There was a problem hiding this comment.
-0.5em works here (or 0 with ±50% translate transform if feeling fancy)
| @@ -122,26 +123,30 @@ 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'); | |||
There was a problem hiding this comment.
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)
Implements #11597 for the website.