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.

Sign up for GitHub

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

Jump to bottom

Simplify each block bindings example #5094

Merged
merged 1 commit into from
Jul 14, 2020
Merged

Simplify each block bindings example #5094

merged 1 commit into from
Jul 14, 2020

Conversation

CyberAP
Copy link
Contributor

@CyberAP CyberAP commented Jul 2, 2020

CSS usage could be avoided here, the same effect can be achieved with a disabled attribute binding.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases, features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests with npm test or yarn test)

@CyberAP
Simplify each block bindings example
e44e504 
No need for CSS here, the same effect could be achieved with just a `disabled` binding.
@PatrickG
Copy link
Member

PatrickG commented Jul 2, 2020

But then you can not uncheck the checkbox anymore,

@CyberAP
Copy link
Contributor Author

CyberAP commented Jul 2, 2020

You can.

antony
antony approved these changes Jul 2, 2020
View reviewed changes
Copy link
Member

@antony antony left a comment

Choose a reason for hiding this comment

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

unless there's a specific reason to demonstrate class here, I think this is reasonable, and more accessible.

@PatrickG
Copy link
Member

PatrickG commented Jul 5, 2020 •
edited
Loading

You can.

How?
You can not uncheck a disabled checkbox.

Oh nevermind. You only disable the text field

@CyberAP
Copy link
Contributor Author

CyberAP commented Jul 5, 2020

You can.

How?
You can not uncheck a disabled checkbox.

Checkbox is not disabled

mindrones
mindrones approved these changes Jul 14, 2020
View reviewed changes
Copy link
Member

@mindrones mindrones left a comment

Choose a reason for hiding this comment

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

Agree too that not having class makes it a bit more encapsulated 👌

@mindrones mindrones merged commit 5c6730e into sveltejs:master Jul 14, 2020
hontas added a commit to hontas/svelte that referenced this pull request Jul 18, 2020
@hontas
Merge remote-tracking branch 'upstream/master'
b1fc702 
* upstream/master: (190 commits)
  invalidate $$props and $$restProps only when there are changes (sveltejs#5123)
  site: use https in link in blog (sveltejs#5148)
  Simplify each block bindings example (sveltejs#5094)
  fix $$props reactive for slots (sveltejs#5125)
  site: add FAQ entry for how to document a svelte component (sveltejs#5131)
  site: remove an obsolete TODO in blog post (sveltejs#5135)
  Increase timeout for unit build
  Increase timeout for unit tests
  -> v3.24.0
  spread condition for input element (sveltejs#5004)
  update changelog
  fix(5018): compare wholeText instead of data (sveltejs#5028)
  html anchor in head (sveltejs#5071)
  error on expression scope store (sveltejs#5079)
  update changelog
  preprocess self-closing script and style tags (sveltejs#5082)
  update changelog
  fix: Parameters with default values are optional (sveltejs#5083)
  make builds time out after a reasonable period (sveltejs#5100)
  site: fix blog typo (sveltejs#5090)
  ...
hontas added a commit to hontas/svelte that referenced this pull request Jul 18, 2020
@hontas
Merge branch 'master' into feature/custom-element-destroy
8fc8c81 
* master: (67 commits)
  add updating guard to binding callback (sveltejs#5126)
  Bump lodash from 4.17.15 to 4.17.19 (sveltejs#5152)
  Bump lodash from 4.17.15 to 4.17.19 in /site (sveltejs#5155)
  Fixes sveltejs#5153 (sveltejs#5154)
  invalidate $$props and $$restProps only when there are changes (sveltejs#5123)
  site: use https in link in blog (sveltejs#5148)
  Simplify each block bindings example (sveltejs#5094)
  fix $$props reactive for slots (sveltejs#5125)
  site: add FAQ entry for how to document a svelte component (sveltejs#5131)
  site: remove an obsolete TODO in blog post (sveltejs#5135)
  Increase timeout for unit build
  Increase timeout for unit tests
  -> v3.24.0
  spread condition for input element (sveltejs#5004)
  update changelog
  fix(5018): compare wholeText instead of data (sveltejs#5028)
  html anchor in head (sveltejs#5071)
  error on expression scope store (sveltejs#5079)
  update changelog
  preprocess self-closing script and style tags (sveltejs#5082)
  ...
Truffula pushed a commit to Truffula/svelte that referenced this pull request Aug 3, 2020
@CyberAP @Truffula
Simplify each block bindings example (sveltejs#5094)
fda1b35 
No need for CSS here, the same effect could be achieved with just a `disabled` binding.
taylorzane pushed a commit to taylorzane/svelte that referenced this pull request Dec 17, 2020
@CyberAP @taylorzane
Simplify each block bindings example (sveltejs#5094)
aa61f12 
No need for CSS here, the same effect could be achieved with just a `disabled` binding.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antony antony antony approved these changes

@mindrones mindrones mindrones approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@CyberAP @PatrickG @antony @mindrones