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.

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

Integrate LuckShiba's timezone command #11

Draft
wants to merge 11 commits into
base: dev
Choose a base branch
from
Draft

Integrate LuckShiba's timezone command #11

wants to merge 11 commits into from

Conversation

MysteryMS
Copy link
Member

CONSEGUI FAZER ESSA BOSTA

@MysteryMS MysteryMS requested a review from Flicksie October 11, 2021 02:53
const tz = ct.lookupViaCity(zoneName)[0]?.timezone || m.tz.zone(zoneName)?.name;

if (!tz) {
msg.channel.send("Couldn't find that place.");
Copy link
Member

Choose a reason for hiding this comment

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

Cloudy command feedback. first-time usage tests were a bit clueless

package.json Outdated Show resolved Hide resolved
Copy link
Member

@Flicksie Flicksie left a comment

Choose a reason for hiding this comment

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

Check inline comments.
Main points are:

  • Improve UX / Response design (blind attempts to use the command return unclear instructions, intuitive usage yields no results)
  • [Minor] Consider the possibility of not using new dependencies for the task.
  • Moment is deprecated internally. Use Intl.DateTimeFormat or date-fns instead

@LuckShiba LuckShiba self-assigned this Oct 26, 2021
@MysteryMS MysteryMS requested a review from Flicksie January 1, 2022 23:00
@MysteryMS MysteryMS marked this pull request as draft June 18, 2022 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants