Repository navigation
Improvements to Soft Navigation docs - #45324
Conversation
|
@chrisdavidmills I know I'm throwing a lot your way at the moment, but if you get a chance could you review since you have the context from #45053 |
chrisdavidmills
left a comment
There was a problem hiding this comment.
A lot of good work here, @tunetheweb. I've looked through it all, and made some suggestions.
Co-authored-by: Chris Mills <chrisdavidmills@gmail.com>
chrisdavidmills
left a comment
There was a problem hiding this comment.
@tunetheweb sorry to be a pain, but...
| ## Instance methods | ||
|
|
||
| This interface overrides the following {{domxref("PerformanceEntry")}} method to include the additional properties: | ||
| This interface overrides the following {{domxref("PerformanceEntry")}} method to include the additional properties added in this interface: |
There was a problem hiding this comment.
I'm sorry about this, but after reading this again, it struck me that I am confused as to what this is trying to day. Can you clarify?
So we are saying that the interface this page covers overrides the toJSON() method to include the properties listed in the "Instance properties" section of this page? What does it mean for a method to include properties? Am I misunderstanding what this is saying?
There was a problem hiding this comment.
So if it just inherited toJSON() from PerformanceEntry then when you called it, it would only include the properties from PerformanceEntry (name, entryType, startTime, duration) and not the additional properties that InteractionContentfulPaint adds on top (interactionId, largestContentfulPaint, paintTime, presentationTime).
So instead of just inheriting toJSON(), it overrides toJSON() to it's own version of that method to ensure the new versions are included (it's basically the same function though — one that iterates through all properties and outputs them in JSON format). This is why it counts as a method on InteractionContentfulPaint and gets it's own page on MDN — because it is a method specific to InteractionContentfulPaint and not just inherited from PerformanceEntry.
Does that make sense?
Can you think about better wording to make this clearer? Maybe this?
| This interface overrides the following {{domxref("PerformanceEntry")}} method to include the additional properties added in this interface: | |
| This interface overrides the {{domxref("PerformanceEntry.toJSON()")}} method to include the additional properties, added in this interface, in the JSON output: |
If that's clearer than can update them all to say that. Or can you suggest better wording?
There was a problem hiding this comment.
Oh, I get it — so it includes its own toJSON(), which replaces the PerformanceEntry version of toJSON(). Its own toJSON() outputs JSON that reflects its object structure, not the object structure of PerformanceEntry.
I kinda took that as read, and didn't even think about it until I started reading this wording, which made me feel a bit confused.
I would write something like:
The
<interface-name>interface overrides the {{domxref("PerformanceEntry.toJSON()")}} method with its own custom version that outputs JSON reflecting its own object structure.
But I would also include it in the method description, rather than as a separate sentence.
Does that work for you?
There was a problem hiding this comment.
I went with this in 3295a8e:
Overrides the
PerformanceEntry.toJSON()method to return a JSON representation of the<interface-name>object.
WDYT?
There was a problem hiding this comment.
Cool, this works fine. I will go through the interface pages, check them, and then get this thing merged.
|
Chris seems to be on top of this. Ping me if you need my review too. |
chrisdavidmills
left a comment
There was a problem hiding this comment.
Very cool. Let's get this in.
|
Thank @chrisdavidmills !! |
Description
This is some tweaks to the docs added in #45053 as @Elchi3 spotted some issues when reviewing the BCD changes.
toJSONmethods. Note thatPerformanceSoftNavigation'stoJSON()exists in Chrome's implementation but was missing from the spec (just added in Specify [Default] object toJSON() for PerformanceSoftNavigation and InteractionContentfulPaint WICG/soft-navigations#91) so is not yet in BCD.navigationIdfromInteractionContentfulPaintandPerformanceSoftNavigationand added it toPerformanceEntryinstead as it's an inherited property.Motivation
Correct docs.
Additional details
Related issues and pull requests
Related PRs: