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

Introduce a DateField type and rename Date component for consistency. #1756

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

chrissnyder2337
Copy link
Contributor

@chrissnyder2337 chrissnyder2337 commented Mar 7, 2024

Introduce a DateField type and rename Date component to follow the naming pattern used by all other JSS field components.

Description / Motivation

Compare Date.tsx to Text.tsx, Image.tsx, Link.tsx, File.tsx, or RichText.tsx. The naming of the Date component is inconsistent with the other field components and it is missing the interface/type like the others have. Ensuring consistency in naming improves the code readability and can reduce issues when developing new components, improving the developer experience.

Testing Details

  • Unit Test Added
  • Manual Test/Other (Please elaborate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Introduce a DateField type and rename Date component to follow the naming pattern used by all other JSS field components. 

Compare Date.tsx to Text.tsx, Image.tsx, Link.tsx, File.tsx, or RichText.tsx. The naming of the Date component is inconsistent with the other field components and it is missing the interface/type like the others have.
@illiakovalenko illiakovalenko added the backlog Issue/PR/discussion is reviewed and added to backlog for the further work label Mar 22, 2024
# Conflicts:
#	packages/sitecore-jss-react/src/components/Date.tsx
@art-alexeyenko
Copy link
Contributor

Hi @chrissnyder2337
I sync'd this propsal with latest changed and checked it in some depth. I think I know why the decision for the naming was taken in favor of DateField. Having it as Date will confict with the built in Date type in typescript. Currently it only fails the component itself (you can see it in the latest changes, one unit test will fail), but renaming for everyone may cause more widespread problems.
With this, I'm inclined to close this PR. Regardless, I appreciate you opening this suggestion and providing a solution - I apologize it takes us a long time to get around to properly reviewing such contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issue/PR/discussion is reviewed and added to backlog for the further work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants