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

fix: fix unexpected CSS overwriting on button element #2383

Merged
merged 1 commit into from
Aug 31, 2024
Merged

Conversation

hsnaydd
Copy link
Contributor

@hsnaydd hsnaydd commented Aug 19, 2024

Fixes: #2377

What's Changed

Fixed unexpected CSS overwriting on the button element

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

@hsnaydd hsnaydd changed the title fix: fix unexpected CSS overwrites fix: fix unexpected CSS overwrites on button element Aug 19, 2024
@hsnaydd hsnaydd changed the title fix: fix unexpected CSS overwrites on button element fix: fix unexpected CSS overwriting on button element Aug 19, 2024
Copy link
Owner

@gpbl gpbl left a comment

Choose a reason for hiding this comment

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

@hsnaydd thanks for your PR. Loving that we are removing extra CSS.

I've been thinking about this and I believe the right way is to rename the selector as .rdp-button-reset and apply this class name to the Button elements used in DayPicker.tsx.

I know this could be more difficult to implement - if you prefer I can go forward by committing in your PR.

src/style.css Outdated
@@ -74,7 +74,7 @@
}

/* Reset buttons */
.rdp-root button {
.rdp-root :where(.rdp-day_button, .rdp-button_previous, .rdp-button_next) {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
.rdp-root :where(.rdp-day_button, .rdp-button_previous, .rdp-button_next) {
.rdp-button-reset

Copy link
Contributor Author

@hsnaydd hsnaydd Aug 19, 2024

Choose a reason for hiding this comment

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

Yes, I thought so too, but then we need to define two classes for the Button component, right? But every component gets its classes from UI enum. How do you plan on adding this class? Do you mean like this?

<components.Button
  type="button"
  className={[classNames[UI.ButtonPrevious], 'button-reset'].join(' ')}
  tabIndex={previousMonth ? undefined : -1}
  disabled={previousMonth ? undefined : true}
  aria-label={labelPrevious(previousMonth, labelOptions)}
  onClick={handlePreviousClick}
>

Copy link
Owner

Choose a reason for hiding this comment

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

@hsnaydd yes, it should come with a relative entry in the UI enum:

className={[classNames[UI.ButtonPrevious], classNames[ButtonReset]].join(' ').trim()}

not so quick as it seems (there are some files to change), but if you feel - give a try!

Copy link
Contributor Author

@hsnaydd hsnaydd Aug 20, 2024

Choose a reason for hiding this comment

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

I don't think this is the right solution. if I want to change the ButtonPrevious class via classNames props ButtonReset will still be applied. If I want to set padding to the ButtonPrevious with my new class I still need to increase the specificity because ButtonReset may overwrite.
Yes, I can set the button_reset to undefined to prevent this but it is not predictable.

Copy link
Owner

Choose a reason for hiding this comment

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

I see what you mean, and I agree that could cause further confusion. Also I like a CSS only solution. We shouldn't use the :where pseudo selector, however, as I'm not sure anymore what will happen for Tailwind users there 🤷🏽

Please could we write everything twice and reset each of the existing buttons?

.rdp-day_button {
  /* existing ..rdp-day_button */
  /* (please remove overrides, if any, below)  */
  border: none;
  background: none;
  padding: 0;
  margin: 0;
  cursor: pointer;
  font: inherit;
  color: inherit;
}

.rdp-button_next,
.rdp-button_previous  {
  /* existing .rdp-button_next, .rdp-button_previous */
  /* (please remove overrides, if any, below)  */
  border: none;
  background: none;
  padding: 0;
  margin: 0;
  cursor: pointer;
  font: inherit;
  color: inherit;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did, but, I don't understand why you want to avoid the :where pseudo selector. What will change for Tailwind users? If you care about the specificity, it is at the same level as what we are writing now. Also if you don't scope with .rdp-root the specificity value will be zero.

Copy link
Owner

Choose a reason for hiding this comment

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

I did, but, I don't understand why you want to avoid the :where pseudo selector. What will change for Tailwind users?

It's just about keeping it simple. I've seen complex selectors may cause confusions to some users (see recent support requests about tailwind).

@gpbl gpbl merged commit 60d071b into gpbl:main Aug 31, 2024
8 checks passed
@gpbl
Copy link
Owner

gpbl commented Aug 31, 2024

@hsnaydd Thanks so much for your work - sorry for the delay.

kodiakhq bot pushed a commit to technifit/tasker that referenced this pull request Sep 8, 2024
This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [react-day-picker](https://daypicker.dev) ([source](https://redirect.github.com/gpbl/react-day-picker)) | [`9.0.8` -> `9.0.9`](https://renovatebot.com/diffs/npm/react-day-picker/9.0.8/9.0.9) | [![age](https://developer.mend.io/api/mc/badges/age/npm/react-day-picker/9.0.9?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/react-day-picker/9.0.9?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/react-day-picker/9.0.8/9.0.9?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/react-day-picker/9.0.8/9.0.9?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>gpbl/react-day-picker (react-day-picker)</summary>

### [`v9.0.9`](https://redirect.github.com/gpbl/react-day-picker/releases/tag/v9.0.9)

[Compare Source](https://redirect.github.com/gpbl/react-day-picker/compare/v9.0.8...v9.0.9)

This release fixes a regression causing the calendar to reset when selecting the days, improves compatibility with the previous version and fixes some other bugs.

#### What's Changed

-   feat: added back more properties to the value returned by `useDayPicker` by [@&#8203;gpbl](https://redirect.github.com/gpbl) in [gpbl/react-day-picker#2427
-   fix: calendar is reset after selecting a day by [@&#8203;gpbl](https://redirect.github.com/gpbl) in [gpbl/react-day-picker#2429
-   fix(style): remove unnecessary styles to buttons in footer in DayPicker by [@&#8203;hsnaydd](https://redirect.github.com/hsnaydd) in [gpbl/react-day-picker#2383
-   fix(style): missing class names for months and years dropdowns [@&#8203;hsnaydd](https://redirect.github.com/hsnaydd) in [gpbl/react-day-picker#2394
-   fix(utilities): `dateMatchModifiers` to use `defaultDateLib` by [@&#8203;gpbl](https://redirect.github.com/gpbl) in [gpbl/react-day-picker#2413
-   fix(types): add `formatWeekNumberHeader` to `Formatters` by [@&#8203;gpbl](https://redirect.github.com/gpbl) in [gpbl/react-day-picker#2412
-   fix(types): add missing `ChevronProps` export by [@&#8203;rishabh-ink](https://redirect.github.com/rishabh-ink) in [gpbl/react-day-picker#2363

#### New Contributors

-   [@&#8203;mahata](https://redirect.github.com/mahata) made their first contribution in [gpbl/react-day-picker#2388
-   [@&#8203;1eeminhyeong](https://redirect.github.com/1eeminhyeong) made their first contribution in [gpbl/react-day-picker#2391
-   [@&#8203;hsnaydd](https://redirect.github.com/hsnaydd) made their first contribution in [gpbl/react-day-picker#2383

**Full Changelog**: gpbl/react-day-picker@v9.0.8...v9.0.9

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] If you want to rebase/retry this PR, check this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/technifit/tasker).
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.

Default styles overwrite button styles used in the footer
2 participants