-
Notifications
You must be signed in to change notification settings - Fork 8
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
Replace narrow non break space with normal space #87
Conversation
function formatDateTime(dateTime: string) { | ||
const narrownNonBreakSpace = String.fromCharCode(8239); | ||
|
||
// Discrepency between server and client, see https://github.com/remix-austin/remixaustin-com/issues/83#issuecomment-1450654389 | ||
return EVENT_TIME_FORMAT.format(new Date(dateTime)).replaceAll( | ||
narrownNonBreakSpace, | ||
" " | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually we can pull this into a util file, but I'd rather wait to do that until I see multiple components/functions using it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd expect there to be a corresponding test for this logic. (also, a nitpick would be to make narrownNonBreakSpace
a constant outside the function somewhere (maybe in the utils?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was being lazy, thanks for calling this out. And yeah, definitely could make that a constant since it doesn't need to be reevaluated on each function call. I suppose I can go ahead and pull this out in a util file so I can easily test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test, if it looks good I'll go ahead and merge.
I wasn't able to figure out a good way to mock Intl.DateTimeFormat to force it to use the narrow non-break space. Didn't feel like it was worth much more time, but if someone has a solution I'm all ears. Otherwise will merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like this solution better. much simpler.
function formatDateTime(dateTime: string) { | ||
const narrownNonBreakSpace = String.fromCharCode(8239); | ||
|
||
// Discrepency between server and client, see https://github.com/remix-austin/remixaustin-com/issues/83#issuecomment-1450654389 | ||
return EVENT_TIME_FORMAT.format(new Date(dateTime)).replaceAll( | ||
narrownNonBreakSpace, | ||
" " | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd expect there to be a corresponding test for this logic. (also, a nitpick would be to make narrownNonBreakSpace
a constant outside the function somewhere (maybe in the utils?)
Pulled it down and it ran great! Thanks! |
A simpler alternative to #84
To test
root.tsx
routes/index.tsx
(since we don't have an upcoming event posted yet)Closes #83