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

[Button]: prevent consumer from overriding disabled atttribute #814

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AlanBreck
Copy link
Collaborator

@AlanBreck AlanBreck commented Sep 4, 2024

With the current configuration, a consumer can disable a button via either <Button isDisabled={true}> or <Button disabled={true}>. However, these can be in conflict, and if the consumer accidentally does something like <Button isDisabled={true} disabled={false}>, then the disabled value wins.

A more likely conflict is if the user sets isLoading to true, but has a different condition for disabled which is false:

<Button isLoading={isFetching} disabled={shouldDisableButtons}>

This PR attempts to avoid this by not allowing the consumer provided disabled attribute to override the disabled state set by the Button component.

Copy link
Contributor

github-actions bot commented Sep 4, 2024

👋 Thanks for Submitting! This PR is available for preview at the link below.

✅ PR tip preview: https://814.pr.nala.bravesoftware.com/
✅ Commit preview: https://814.pr.nala.bravesoftware.com/commit-2fb732ad11dd8fbaa31618de14e6828cfc56ccd0/

- ./tokens/css/variables-android.old.css: 7424 bytes
+ ./tokens/css/variables-android.css: 7424 bytes
---
- ./tokens/css/variables-browser.old.css: 6874 bytes
+ ./tokens/css/variables-browser.css: 6874 bytes
---
- ./tokens/css/variables-ios.old.css: 7067 bytes
+ ./tokens/css/variables-ios.css: 7067 bytes
---
- ./tokens/css/variables-marketing.old.css: 13561 bytes
+ ./tokens/css/variables-marketing.css: 13561 bytes
---
- ./tokens/css/variables-news.old.css: 526 bytes
+ ./tokens/css/variables-news.css: 526 bytes
---
- ./tokens/css/variables-newtab.old.css: 1941 bytes
+ ./tokens/css/variables-newtab.css: 1941 bytes
---
- ./tokens/css/variables-search.old.css: 2419 bytes
+ ./tokens/css/variables-search.css: 2419 bytes
---
- ./tokens/css/variables-web3.old.css: 905 bytes
+ ./tokens/css/variables-web3.css: 905 bytes
---
- ./tokens/css/variables.old.css: 114834 bytes
+ ./tokens/css/variables.css: 114834 bytes
Variables Diff: variables-android.diff
--- ./tokens/css/variables-android.old.css	2024-09-04 19:09:10.770362866 +0000
+++ ./tokens/css/variables-android.css	2024-09-04 19:08:33.082242310 +0000
@@ -1,6 +1,6 @@
 /**
  * Do not edit directly
- * Generated on Wed Sep 04 2024 13:09:36 GMT+0000 (Coordinated Universal Time)
+ * Generated on Wed Sep 04 2024 19:08:33 GMT+0000 (Coordinated Universal Time)
  */
 
 :root {
Variables Diff: variables-browser.diff
--- ./tokens/css/variables-browser.old.css	2024-09-04 19:09:10.978363276 +0000
+++ ./tokens/css/variables-browser.css	2024-09-04 19:08:33.058242225 +0000
@@ -1,6 +1,6 @@
 /**
  * Do not edit directly
- * Generated on Wed Sep 04 2024 13:09:36 GMT+0000 (Coordinated Universal Time)
+ * Generated on Wed Sep 04 2024 19:08:33 GMT+0000 (Coordinated Universal Time)
  */
 
 :root {
Variables Diff: variables-ios.diff
--- ./tokens/css/variables-ios.old.css	2024-09-04 19:09:11.242363542 +0000
+++ ./tokens/css/variables-ios.css	2024-09-04 19:08:33.098242366 +0000
@@ -1,6 +1,6 @@
 /**
  * Do not edit directly
- * Generated on Wed Sep 04 2024 13:09:36 GMT+0000 (Coordinated Universal Time)
+ * Generated on Wed Sep 04 2024 19:08:33 GMT+0000 (Coordinated Universal Time)
  */
 
 :root {
Variables Diff: variables-marketing.diff
--- ./tokens/css/variables-marketing.old.css	2024-09-04 19:09:11.442363741 +0000
+++ ./tokens/css/variables-marketing.css	2024-09-04 19:08:33.122242450 +0000
@@ -1,6 +1,6 @@
 /**
  * Do not edit directly
- * Generated on Wed Sep 04 2024 13:09:36 GMT+0000 (Coordinated Universal Time)
+ * Generated on Wed Sep 04 2024 19:08:33 GMT+0000 (Coordinated Universal Time)
  */
 
 :root {
Variables Diff: variables-news.diff
--- ./tokens/css/variables-news.old.css	2024-09-04 19:09:11.686363985 +0000
+++ ./tokens/css/variables-news.css	2024-09-04 19:08:33.146242534 +0000
@@ -1,6 +1,6 @@
 /**
  * Do not edit directly
- * Generated on Wed Sep 04 2024 13:09:36 GMT+0000 (Coordinated Universal Time)
+ * Generated on Wed Sep 04 2024 19:08:33 GMT+0000 (Coordinated Universal Time)
  */
 
 :root {
Variables Diff: variables-newtab.diff
--- ./tokens/css/variables-newtab.old.css	2024-09-04 19:09:11.874364173 +0000
+++ ./tokens/css/variables-newtab.css	2024-09-04 19:08:33.154242562 +0000
@@ -1,6 +1,6 @@
 /**
  * Do not edit directly
- * Generated on Wed Sep 04 2024 13:09:36 GMT+0000 (Coordinated Universal Time)
+ * Generated on Wed Sep 04 2024 19:08:33 GMT+0000 (Coordinated Universal Time)
  */
 
 :root {
Variables Diff: variables-search.diff
--- ./tokens/css/variables-search.old.css	2024-09-04 19:09:12.070364370 +0000
+++ ./tokens/css/variables-search.css	2024-09-04 19:08:33.138242506 +0000
@@ -1,6 +1,6 @@
 /**
  * Do not edit directly
- * Generated on Wed Sep 04 2024 13:09:36 GMT+0000 (Coordinated Universal Time)
+ * Generated on Wed Sep 04 2024 19:08:33 GMT+0000 (Coordinated Universal Time)
  */
 
 :root {
Variables Diff: variables-web3.diff
--- ./tokens/css/variables-web3.old.css	2024-09-04 19:09:12.242364542 +0000
+++ ./tokens/css/variables-web3.css	2024-09-04 19:08:33.158242576 +0000
@@ -1,6 +1,6 @@
 /**
  * Do not edit directly
- * Generated on Wed Sep 04 2024 13:09:36 GMT+0000 (Coordinated Universal Time)
+ * Generated on Wed Sep 04 2024 19:08:33 GMT+0000 (Coordinated Universal Time)
  */
 
 @media (prefers-color-scheme: light) {
Variables Diff: variables.diff
--- ./tokens/css/variables.old.css	2024-09-04 19:09:12.478364778 +0000
+++ ./tokens/css/variables.css	2024-09-04 19:08:32.926241762 +0000
@@ -1,6 +1,6 @@
 /**
  * Do not edit directly
- * Generated on Wed Sep 04 2024 13:09:36 GMT+0000 (Coordinated Universal Time)
+ * Generated on Wed Sep 04 2024 19:08:32 GMT+0000 (Coordinated Universal Time)
  */
 
 :root {

Copy link
Collaborator

@fallaciousreasoning fallaciousreasoning 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 this is a good argument for just using the same property name as the default one on the button next time 😆

@AlanBreck
Copy link
Collaborator Author

If we had some way of deprecating properties, we could do that now, right?

@fallaciousreasoning
Copy link
Collaborator

As a comment on the export, maybe?

/**
 * Deprecated - use disabled instead
 * /
export let isDisabled = false;

@AlanBreck
Copy link
Collaborator Author

Unfortunately, there's no great path for deprecation right now apart from a console log if it's detected, but a moderator from the Svelte Discord channel kindly created an issue on sveltejs/language-tools requesting support for @deprecated: sveltejs/language-tools#2491

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.

2 participants