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

Calendar: TypeScript Definitions do not match with actual PT props #6094

Closed
vaelu opened this issue Mar 6, 2024 · 8 comments · Fixed by #6096
Closed

Calendar: TypeScript Definitions do not match with actual PT props #6094

vaelu opened this issue Mar 6, 2024 · 8 comments · Fixed by #6096
Assignees
Labels
Typescript Issue or pull request is *only* related to TypeScript definition
Milestone

Comments

@vaelu
Copy link
Contributor

vaelu commented Mar 6, 2024

Describe the bug

We noticed, that some PT props have to be written lowercase instead of camelCase that they work.

Specifically in calendar component:

  • previousButton (works only when written as previousbutton)
  • nextButton (works only when written as nextbutton)
  • tableHeaderCell (works only when written as tableheadercell)
  • weekDay (works only when written as weekday)
  • dayLabel (works only when written as daylabel)
  • monthPicker (works only when written as monthpicker)
  • yearPicker (works only when written as yearpicker)
  • timePicker (works only when written as timepicker)
  • separatorContainer (works only when written as separatorcontainer)
  • hourPicker (works only when written as hourpicker)
  • minutePicker (works only when written as minutepicker)
  • ampmPicker (works only when written as ampmpicker)
  • incrementButton (works only when written as incrementbutton)
  • decrementButton (works only when written as decrementbutton)
  • groupContainer (works only when written as groupcontainer)

In the passthrough provided directly in the repo (https://github.com/primefaces/primereact/blob/master/components/lib/passthrough/tailwind/index.js), the properties are the "correct" way (so that they work, but it's wrong for TypeScript), but in the docs not.

Reproducer

No response

PrimeReact version

10.5.1

React version

18.x

Language

TypeScript

Build / Runtime

Vite

Browser(s)

No response

Steps to reproduce the behavior

No response

Expected behavior

No response

@vaelu vaelu added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Mar 6, 2024
@melloware melloware added Typescript Issue or pull request is *only* related to TypeScript definition and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Mar 6, 2024
melloware added a commit to melloware/primereact that referenced this issue Mar 6, 2024
@melloware melloware self-assigned this Mar 6, 2024
@melloware melloware added this to the 10.5.2 milestone Mar 6, 2024
@melloware
Copy link
Member

PR submitted. I had fixed this before and another commit later accidentally overwrote these changes

@vaelu
Copy link
Contributor Author

vaelu commented Mar 6, 2024

Thanks. But are you sure that this will fix the problem? You just changed the default PT to match the typescript definitions, but the main problem is that the PTs I mentioned above are not respected by the calendar component, when written in the correct typescript-case.

@melloware
Copy link
Member

they should be. lets take the real code

Calendar Typescript is previousButton

previousButton?: CalendarPassThroughType<React.HTMLAttributes<HTMLButtonElement>>;

Calendar PassThrough PTM is previousButton

const previousButtonProps = mergeProps(
{
type: 'button',
className: cx('previousButton'),
'aria-label': previousButtonLabel,
...navigatorProps
},
ptm('previousButton')

Finally now that Tailwind index will match that case and instead of being previousbutton is previousButton right?

@melloware
Copy link
Member

See this working example of previousButton https://stackblitz.com/edit/s2zggm?file=src%2FApp.jsx

@vaelu
Copy link
Contributor Author

vaelu commented Mar 6, 2024

Possible, because we merge the default props with our custom ones. Will try when the new version is out. Or can I test it somehow beforehand?

@melloware
Copy link
Member

you can probably do a local build with npm run build:lib ?

@vaelu
Copy link
Contributor Author

vaelu commented Mar 7, 2024

Thanks. I added a comment to the PR.

However, I still don't understand the following behaviour:
For your understanding, we merge the default PT from PrimeReact with our own (not with tailwind-merge, just merging the two objects) like this:

import { Calendar as PCalendar, type CalendarProps as PCalendarProps } from "primereact/calendar";
import TailwindDefaultPT from "primereact/passthrough/tailwind";
import InputDatePT from "./input-date-pt";

export const InputDate = (inputDateProps: PCalendarProps) => {
  const props = { ...inputDateProps };
  return <PCalendar unstyled pt={{ ...TailwindDefaultPT.calendar, ...InputDatePT }} {...props} />;
};

As you said, the component should take previousButton (camel-case):

const previousButtonProps = mergeProps(
{
type: 'button',
className: cx('previousButton'),
'aria-label': previousButtonLabel,
...navigatorProps
},
ptm('previousButton')

However, when a previousbutton key exists (like it did before because the default PT was wrong), the component will take this instead of previousButton.

@melloware
Copy link
Member

melloware commented Mar 7, 2024

So from my PR and looking at the code there is no lowercase previousbutton anywhere anymore so where is it coming from?

Can you put together a Stack Blitz showing the issue?

nitrogenous added a commit that referenced this issue Mar 14, 2024
Fix #6094: Calendar Tailwdind Passthrough
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typescript Issue or pull request is *only* related to TypeScript definition
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants