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

no id:scoped for repeated w-id #1377

Merged
merged 1 commit into from Jul 3, 2019
Merged

no id:scoped for repeated w-id #1377

merged 1 commit into from Jul 3, 2019

Conversation

mlrawlings
Copy link
Member

Description

In Marko 3, w-id was used as both a key and set a scoped id. The id was primarily an implementation detail, but it could also be used with w-for to associate DOM elements.

Marko 3:

<label w-for="foo"/>
<input w-id="foo"/>

Marko 4 equivalent:

<label for:scoped="foo"/>
<input key="foo" id:scoped="foo"/>

However Marko 4's :scoped modifier does not support repeated values. This is because it's difficult to reason about association of repeated values (the primary use-case fore :scoped). However, this causes the same ids to appear on the page because :scoped doesn't have special behavior for repeated[] values.

In Marko 3, this association doesn't actually work between repeated w-id and w-for attributes:

Input template:

<label w-for="foo[]">1</label>
<input w-id="foo[]" value="1">
<label w-for="foo[]">2</label>
<input w-id="foo[]" value="2">

Output HTML:

<label for="foo[0]">1</label>
<input id="foo[1]" value="1">
<label for="foo[2]">2</label>
<input id="foo[3]" value="2">

Because of this, it should be safe to assume that a repeated w-id is only being used to reference the elements, not associate them with other elements. This PR modifies the migration of w-id so that repeated ids do not add an id:scoped attribute.

Checklist:

  • I have read the CONTRIBUTING document and have signed (or will sign) the CLA.
  • I have updated/added documentation affected by my changes.
  • I have added tests to cover my changes.

@codecov
Copy link

codecov bot commented Jul 3, 2019

Codecov Report

Merging #1377 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1377      +/-   ##
==========================================
+ Coverage   90.59%   90.59%   +<.01%     
==========================================
  Files         341      341              
  Lines       12467    12468       +1     
==========================================
+ Hits        11294    11295       +1     
  Misses       1173     1173
Impacted Files Coverage Δ
src/core-tags/migrate/all-tags/w-id.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 535daf9...a80ad07. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0008%) to 90.592% when pulling a80ad07 on w-id-repeated into 535daf9 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0008%) to 90.592% when pulling a80ad07 on w-id-repeated into 535daf9 on master.

@mlrawlings mlrawlings merged commit 676a6b7 into master Jul 3, 2019
@DylanPiercey DylanPiercey deleted the w-id-repeated branch July 8, 2019 17:23
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.

None yet

2 participants