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

feat(clock): small analogue clock face #54

Merged
merged 13 commits into from
Nov 3, 2021
Merged

Conversation

bualoy-napat
Copy link
Collaborator

@bualoy-napat bualoy-napat commented Oct 28, 2021

Description

A new feature on the clock. When the width is less than 130px, the face of the clock will change to a smaller face that is easy to read.

Update UI of digital and analog clock to match with current spec from UX.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have checked my code and corrected any misspellings

@bualoy-napat bualoy-napat added the enhancement New feature or request label Oct 28, 2021
@bualoy-napat bualoy-napat self-assigned this Oct 28, 2021
Copy link
Collaborator

@dtanp-rft dtanp-rft left a comment

Choose a reason for hiding this comment

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

I'd like to do another review after comments have been addressed.

packages/elements/src/clock/index.ts Outdated Show resolved Hide resolved
packages/elements/src/clock/index.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@dtanp-rft dtanp-rft left a comment

Choose a reason for hiding this comment

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

I think size=small should not be added if it's not analog clock.
Would you please change it and write test for this change?

Regarding to the test, I suggest you breakdown test case to be smaller test. When test is failed, it's easier to understand what is going wrong e.g.

Shows small size clock when width is less than 130px

  1. test if [part=digital] is rendered if width >= 130px
  2. set width < 130px
  3. test if [part=digital] is not rendered
  4. test if [part="segment am-pm"] is rendered (ps. I wouldn't bother to check if it needs to be inside [part="hands"] but it's fine if you want the test is a little bit strict

Small size clock show AM/PM if it has attribute am-pm

  1. Use el = await fixture('');
  2. Set width <= 130px
  3. Test if am-pm part is rendered

Attribute size=small shouldn't show if it's not analog clock

  1. el = await fixture('');
  2. Set width < 130px
  3. Test if size=small is not present
  4. Set width >= 130px
  5. Test if size=small is not present

@dtanp-rft dtanp-rft self-requested a review November 3, 2021 06:51
Copy link
Collaborator

@dtanp-rft dtanp-rft left a comment

Choose a reason for hiding this comment

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

Please squash commit when merge.

@bualoy-napat bualoy-napat merged commit bc4b2a8 into develop Nov 3, 2021
@bualoy-napat bualoy-napat deleted the simple-clock-face branch November 3, 2021 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants