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

Use one-way bindings for wpt-metadata and wpt-amend-metadata #4187

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

past
Copy link
Member

@past past commented Jan 4, 2025

This migration was a bit larger than usual due to the fact that the metadata component and the triage component interact so closely. The main changes are:

  • Removal of notify: true and change double angle brackets to double brackets.
  • Use get/set to let Polymer properly detect array property updates.
  • DOM manipulation logic is now encapsulated within methods to better adhere to Polymer's data flow principles.

@@ -100,7 +100,7 @@ class WPTMetadata extends PathInfo(LoadingState(PolymerElement)) {
></wpt-metadata-node>
</template>
</iron-collapse>
<paper-button id="metadata-toggle" onclick="[[openCollapsible]]">
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this variable should be removed in the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -195,7 +195,7 @@ class TestFileResultsTable extends WPTFlags(Pluralizer(AmendMetadataMixin(WPTCol
</template>
</tbody>
</table>
<wpt-amend-metadata id="amend" selected-metadata="{{selectedMetadata}}" path="[[path]]"></wpt-amend-metadata>
<wpt-amend-metadata id="amend" selected-metadata="[[selectedMetadata]]" path="[[path]]"></wpt-amend-metadata>
Copy link
Collaborator

Choose a reason for hiding this comment

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

we still need to listen to the change in selectedMetadata in test-file-results-table.js, because it is used to clear the selected cells UI when the triage UI is dismissed through clearSelectedCells. I will post a recorded video clips to demonstrate that

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! It's actually the method in wpt-results.js that clears the cells, but I changed it here for consistency.

triage-notifier="[[triageNotifier]]"></wpt-metadata>
</template>
<wpt-amend-metadata id="amend" selected-metadata="{{selectedMetadata}}" path="[[path]]"></wpt-amend-metadata>
<wpt-amend-metadata id="amend" selected-metadata="[[selectedMetadata]]" path="[[path]]"></wpt-amend-metadata>
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto. Still need a EventListener for selectedMetadata

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent point, fixed.

metadata-map="{{metadataMap}}"
label-map="{{labelMap}}"
metadata-map="[[metadataMap]]"
label-map="[[labelMap}]]"
Copy link
Collaborator

Choose a reason for hiding this comment

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

wpt-results still need to add EventListener for metadataMap and labelMap because the data is being fetched inside of . Once it is fetched, notifies wpt-results that data has been fetched. Then wpt-results updates the inline labels UI in the test results.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, done.

@KyleJu
Copy link
Collaborator

KyleJu commented Jan 18, 2025

Screen.recording.2025-01-17.4.15.36.PM.webm

How the upward data binding for selectedMetadata is being used. The first example is wpt.fyi prod and the second example is this PR (which is broken in this change).

@past
Copy link
Member Author

past commented Jan 23, 2025

This is ready now for another review pass.

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