-
Notifications
You must be signed in to change notification settings - Fork 474
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
Fix #69 - Render BCD tables #85
Conversation
I haven't tried it yet but I'm in awe of your contribution @joedarc |
By the way @joedarc I know our SCSS is in flux but if you're looking for another interesting issue to work on take a look at: #44 Basically, we need to have little "chain icons" near the headlines that are clickable and share'able so you can link to headlines. That issue talks a lot about avoiding expensive DOM mutations but I hope the gist is there. I.e. we want something that allows you to link to individual sections and ideally we shouldn't need to do weird and expensive tricks to make that possible. We can chat more about the details in the issue (or your PR) if you would be interested in looking into this. |
Yeah I'll stick around and make adjustments as needed for it, there very well could be something i missed, and I'd be happy to fix whenever. |
Note the little close (x) icon on floating right. I don't think we really need that since the little arrow you clicked to expand it was already found by the user and it's highlit so it's easy to find if you want to close it. Note the |
I tested this on iOS Safari and it's pretty busted but I think that's because we don't really have a CSS framework yet. In particular, the whole left sidebar ought to disappear on a small screen (and show on the bottom?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've already butchered this enough as it is. Have a look at some of the early feedback.
Also, I found the names of files quite overbearing. Perhaps the main component file could be called index.js
inside the client/src/ingredients/browser-compatibility-table/
folder and then inside that folder you can rename things browser-compatibility-platforms.js
to platforms.js
client/src/document.js
Outdated
@@ -5,6 +5,7 @@ import { NoMatch } from "./routing"; | |||
import { InteractiveExample } from "./ingredients/interactive-example"; | |||
import { Attributes } from "./ingredients/attributes"; | |||
import { Examples } from "./ingredients/examples"; | |||
import BrowserCompatibilityTable from "./ingredients/browser-compatibility-table/browser-compatibility-table"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid default exports. Notice that all the other components, on the lines above, prefer named exports. (or named imports I guess)
"webextensions-mobile": ["firefox_android"] | ||
}; | ||
|
||
export default class BrowserCompatibilityTable extends Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export default class BrowserCompatibilityTable extends Component { | |
export class BrowserCompatibilityTable extends Component { |
} | ||
|
||
componentDidMount() { | ||
// This is where we can fetch the compatibility data and set state to render the table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole component should be stateless. In particular, since all the other "ingredients" are. (The word "ingredients" comes from the fact that we used to have the concept of "recipes" which meant a recipe decides which "things" to put into a page and each "thing" became called an ingredient.
Perhaps a better name for this folder of React components should be stateless. Or, display components. Or thing showers. :)
Either way, stateless components isn't a good name either because some state is OK. For example your neat currentNoteId
idea.
But copying the whole core data (aka. props.document.browser_compatibility
) into state is incorrect. E.g. this blog post
I've been bitten by this before and it's dangerous especially as the component re-mounts.
So, I think you need to rewrite this component a bit:
From, document.js
it should call it like this:
<BrowserCompatibilityTable key="browser_compatibility" data={document.browser_compatibility} />
Note the removal of a default value for category
. It's better to do something like const category = this.props.category || 'html';
And then the constructor here should look like this:
export default class BrowserCompatibilityTable extends Component {
state = {
currentNoteId: null,
hasDeprecation: false,
hasExperimental: false,
hasNonStandard: false,
hasFlag: false,
hasPrefix: false,
hasNotes: false,
legendSet: false
}
render() {
const {data, category = 'html' } = this.props;
...
<tbody>
<BrowserCompatibilityRows
compatibilityData={data}
...
}
}
|
||
render() { | ||
if (!this.state.compatibilityData) { | ||
return <div /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to the nature of this whole component being just about displaying stuff (aka. displaying ingredients) I think it's a good idea to assume that the data is always truthy.
But I can see benefits in keeping a check like this. It'll help debugging and unit testing. E.g.
render() {
const {data, category = 'html' } = this.props;
if (!data || !Object.keys(data).length) {
throw new Error("BrowserCompatibilityTable component called with empty data");
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, I think that somewhere in document.js
we'll do something like this:
// sort-of pseudo code
if (document.browser_compatibility) {
<h2>Browser Compatibilty</h2>
<BrowserCompatibilityTable data={document.browser_compatibility}/>
} else {
<em>This reference page does not have Browser Compatibility.</em>
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope that pseduo code makes sense. Thing is, and I've mentioned this elsewhere, the BCD data might be lazy-loaded and then it might look something like this:
class Document extends Component {
state = {
bcdData: null,
bcdDataLoaded: false,
bcdDataError: null
}
async componentDidMount() {
if (!bcdDataLoaded) {
try {
const response = await fetch('./bcd.json')
if (!response.ok) {
this.setState({bcdDataError: response})
} else {
const bcdData = await response.json
this.setState({bcdData });
}
} catch (err) {
this.setState({bcdDataError: err})
}
}
}
render() {
const {bcdData} = this.state;
const {document} = this.props;
return <div>
<h1>{document.title}</h1>
<div>{document.introduction}</div>
<RenderExamples examples={document.examples}/>
{bcdData && <BrowserCompatibiltyTable data={bcdData}/>}
<div>
<h3>Contributors</h3>
<RenderContributors contributors={document.contributors}/>
</div>
</div>
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ Just an example but it lends itself to something like this:
loadBCDData = async () => {
try {
const response = fetch('./bcd.json')
...
}
render() {
const {bcdData} = this.state;
const {document} = this.props;
return <div>
<h1>{document.title}</h1>
<div>{document.introduction}</div>
<RenderExamples examples={document.examples}/>
{!bcdData && !bcdDataError && <IntersectionObserverWrapper onView={this.loadBCDData}/>}
{bcdData && <BrowserCompatibiltyTable data={bcdData}/>}
<div>
<h3>Contributors</h3>
<RenderContributors contributors={document.contributors}/>
</div>
</div>
}
import { BrowserCompatibilityRows } from "./browser-compatibility-rows"; | ||
import { BrowserCompatibilityLegend } from "./browser-compatibility-legend"; | ||
|
||
const browsers = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const browsers = { | |
const BROWSERS = { |
?? just a nit but UPPERCASING stuff usually sends a message to the code-reader that this is a proper constant.
hasNotes: false, | ||
legendSet: false | ||
} | ||
this.onNotesClick = this.onNotesClick.bind(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to do this since we're running on top of create-react-app which ships with a Babel that's configured to support public field methods. So you can remove this line and rewrite...
onNotesClick(currentNoteId) {
if (this.state.currentNoteId ...
...to...
onNotesClick = currentNoteId => {
if (this.state.currentNoteId ...
@@ -77,4 +77,614 @@ div.content { | |||
.live-sample-frame { | |||
max-width: 42rem; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you cut all the additions inside mdn.scss
and put them into a file called bcd.scss
and then import it from the top of document.js
like so:
import('./bcd.scss');
Eventually, we might lazy-load that CSS too so it's only loaded lazily on documents that have BCD tables. But this'll tie us over. Also, when time allows @schalkneethling will take over that file and do whatever magic he needs to do with it.
client/src/mdn.scss
Outdated
} | ||
|
||
.bc-table .bc-platforms .bc-platform-desktop:before { | ||
background-image: url("https://developer.mozilla.org/static/platforms/desktop.d6def92f82da.svg"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
background-image: url("https://developer.mozilla.org/static/platforms/desktop.d6def92f82da.svg"); | |
background-image: url("/bcd/platforms/desktop.svg"); |
And copy the SVGs from https://github.com/mozilla/kuma/tree/master/jinja2/includes/icons/platforms into client/public/bcd
or something.
function RenderBrowserSupportDetails({browserSupportDetails, rowIndex, indexNotes, currentNoteId, onNotesClick}) { | ||
return browserSupportDetails.map((browserSupportDetail, detailIndex) => | ||
<BrowserSupportDetail | ||
key={`${rowIndex}-${detailIndex}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to avoid building keys from integers. It's extremely error-prone and can lead to very subtle bugs.
Since Stumptown is a SPA, the BCD table might mount once and re-rendered with different document.browser_compatibility
structs and this can lead to incorrect re-renders.
Here's a good example: https://medium.com/@vraa/why-using-an-index-as-key-in-react-is-probably-a-bad-idea-7543de68b17c
Try to come up with something unique instead. To be honest I don't really know what browserSupportDetail
is but perhaps you can use key={this.computeDistinctKey(browserSupportDetail)}
and write a little function that does something like...
computeDistinctKey(detail) {
return `${detail.browser}:${detail.support}:${detail.version_added}`
}
} else if (indexNote.notes.length > 0) { | ||
browserSupportNotes.push( | ||
indexNote.notes.map((note, index) => { | ||
currentNoteContent = `<abbr class="only-icon" title="See implementation notes"><span>Notes</span><i class="ic-footnote"></i></abbr> ${note}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't really see what's going on or if this is just a matter of prototype-stage, but why isn't this:
const currentNoteContent = <><abbr className="only-icon" title="See implementation notes"><span>Notes</span><i className="ic-footnote"></i></abbr> {note}</>;
It would definitely avoid the dangerouslySetInnerHTML
in BrowserSupportNoteContent
and it means we don't have to worry about the note
string, from the BCD data, being something like This is an innocent note <script>alert(xss)</script>
MOST IMPORTANTLY; THANK YOU! This is amazing progress. I know I peppered you with nits and feedback and I'm sorry if I patronized with any "lecturing" which might stem from getting an early prototype into review. Having this is going to be amazing. The code quality is going to matter since this is our chance to do BCD tables properly and correctly rather than the organic thing that lives in Kuma. |
@peterbe thank you for all the feedback! Admittedly some of this was "get this up there so i can see if this is what they want or completely in the wrong direction."
Yeah that was the reason i put the compatibility data into state because i figured on mount it would fetch that specific data, but i definitely understand what you're getting at. Most of the changes shouldn't take too long, I'll get to them shortly. Thanks again for your time reviewing! |
@peterbe, pretty sure i covered all the bases here. Let me know! |
return ' To change preferences in Chrome, visit chrome://flags.'; | ||
case 'firefox': | ||
case 'firefox_android': | ||
return ' To change preferences in Firefox, visit about:config.'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are eventually going to have to go into a gettext
functionality so they can be localized. So you'll need to remove that extra prefix whitespace.
@@ -32,7 +45,7 @@ export function BrowserSupportNotes({ indexNote, blockElementTag, noteElementTag | |||
} else if (indexNote.notes.length > 0) { | |||
browserSupportNotes.push( | |||
indexNote.notes.map((note, index) => { | |||
currentNoteContent = `<abbr class="only-icon" title="See implementation notes"><span>Notes</span><i class="ic-footnote"></i></abbr> ${note}`; | |||
currentNoteContent = <><abbr className="only-icon" title="See implementation notes"><span>Notes</span><i className="ic-footnote"></i></abbr><span dangerouslySetInnerHTML={{__html: note}}></span></>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these note
strings that have to contain HTML strings? It makes me nervous. dangerouslySetInnerHTML
really is dangerous because if someone manages to trick the BCD data to be something very different, we have a security risk.
The blobs of HTML we get from the prose in stumptown-content is also dangerous but it's something we're very aware of and there's plenty (at least will be) scripting that asserts that they're always safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example: Defaults to metadata
in Chrome 64.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up adding in https://github.com/remarkablemark/html-react-parser to handle the markdown in the notes. Let me know if you're alright with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that is not correct. The renderer shouldn't need to do that kind of work. That's what the build-json
script in stumptown-content
is supposed to own.
If the BCD content in the JSON files that it picks up from stumptown/packaged/**/*.json
isn't right, then let's hop over to https://github.com/mdn/stumptown-content/issues/new and resolve it there.
File the issue and to avoid losing steam come back here and write in the issue URL as a code comment.
Right, @wbamberg ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build-json just fetches the relevant BCD content from browser-compat-data, given the BCD query string in the stumptown content. It does not do any processing on the BCD content.
The BCD content is JSON, but notes and descriptions are allowed to use a small subset of HTML tags (see e.g. https://github.com/mdn/browser-compat-data/blob/master/schemas/compat-data-schema.md#notes) and should be treated as HTML (as they already are in Kuma, via the Compat.ejs macro). BCD content should be at least as safe as stumptown content.
But I'm puzzled by "to handle the markdown in the notes". I don't think the notes should contain Markdown, can you show me where that happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification @joedarc !
The blobs of HTML we get from the prose in stumptown-content is also dangerous but it's something we're very aware of and there's plenty (at least will be) scripting that asserts that they're always safe.
As this implies, we already use dangerouslySetInnerHTML
for the other HTML content, and I'd say that BCD content is at least as safe as stumptown-content.
But there is a general question about what to do about HTML we receive from relatively-trustworthy-sources like BCD and stumptown-content. Both these sources should have their own sanitizing but the renderer might want to sanitize as well, just in case.
I think it's @peterbe 's decision how to render content, but my 2c: we should use dangerouslySetInnerHTML
here, but sanitize the content first, and we should do the same thing anywhere else we use dangerouslySetInnerHTML
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely dangerouslySetInnerHTML
in favor of any runtime moving parts that could be avoided. For example, if we, for some reason, can't trust this HTML to be dangerously injected we can write a thing in the stumptown-renderer/cli
that does the heavy lifting of making us more comfortable.
My fear is that I'm not familiar with how the BCD pipeline and checks-and-balances work. If someone "sneaks" in a PR on BCD and changes the note there to Defaults to <code>metadat</code> in Chrome <script>alert(xss)</script>
without it being caught by humans or automation there.
For the sake of this PR, just use dangerouslySetInnerHTML
and we'll file some issues about investigating or writing extra checks that makes sure BCD and stumptown-content are safe to dangerously inject.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, it is my intention to write a tool that ships stumptown-content pull requests into its own staging subdomains. First of all, those subdomains will never be hosted on .mozilla.org
anyway.
Second, if a PR is turned into a fully working subdomain (e.g. pr-1234.stumptown-staging.io) it means it would build without a human approver and that'd make a great test of trying to break the system. The second worst that can happen is that someone writes a rogue site that hacks into all *.stumptown-staging.io
pages. The worst thing that can happen is that someone manages to put up a .stumptown-staging.io
site installs malware into unsuspecting browsers. I.e. "Here go to this Mozilla site. It's fine. It's made by Muzilla."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of this PR, just use dangerouslySetInnerHTML
Reverted back to the original implementation per request. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My fear is that I'm not familiar with how the BCD pipeline and checks-and-balances work. If someone "sneaks" in a PR on BCD and changes the note there to
Defaults to <code>metadat</code> in Chrome <script>alert(xss)</script>
without it being caught by humans or automation there.
I'm happy to prioritize fixing this on the BCD side.
@@ -75,7 +88,7 @@ export function BrowserSupportNotes({ indexNote, blockElementTag, noteElementTag | |||
if (indexNote.alternatives.length > 0) { | |||
browserSupportNotes.push( | |||
indexNote.alternatives.map((alternative, index) => { | |||
currentNoteContent = `<abbr class="only-icon" title="Uses the non-standard name: <code>${alternative.alternative_name}</code>"><span>Alternate Name</span><i class="ic-altname"></i></abbr> Uses the non-standard name: <code>${alternative.alternative_name}</code>` | |||
currentNoteContent = <><abbr className="only-icon" title={`Uses the non-standard name: <code>${alternative.alternative_name}</code>`}><span>Alternate Name</span><i className="ic-altname"></i></abbr> Uses the non-standard name: <code>{alternative.alternative_name}</code></> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTML tags in a tooltip? Was this deliberate? (not a security thing, but still a bit weird)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually took this from an example at https://developer.mozilla.org/en-US/docs/Web/HTML/Element/audio, but you're right..that is weird..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the tags from the tooltip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell, BCD would like to remove these things in the source, too: mdn/browser-compat-data#4033 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah i see, yeah i tried to keep all of this in-line with what is currently out there in the wild. Makes sense to remove it though.
Quick heads-up. I have not forgotten the review of this PR. Just struggling to find the time. |
Sounds good. Let me know if there’s any other fixes you want to see here before you land it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's glorious to see it working and looking just as great as the current BCD table on developer.mozilla.org.
I'm not entirely done with my review because I ran out of time but I made it half-way through.
client/src/bcd.scss
Outdated
@@ -0,0 +1,637 @@ | |||
div.content { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't great. Is there a way around it? The output of this looks like this grep on client/build/static/css/main.7365d324.chunk.css
:
If it's really necessary to have this prefix for every single selector, can we make it a bit more specific like div.bcd-table {
or something? (you can see the whole compiled CSS prettified here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this was probably a result of me copying it from mdn.scss at the start. Most of the names include bc-
so i just removed the content wrap. There still might be a few styles that can be moved into mdn.scss.
client/src/bcd.scss
Outdated
display: inline-block; | ||
content: ''; | ||
background-repeat: no-repeat; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these here .external-icon
are pretty and useful but I don't think they have anything to do with BCD. So can you move them to mdn.scss
and move the external-link.svg
to some folder other than "bcd". E.g. client/public/images
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll move them. The only place in BCD it is used is for the "Update compatibility data on GitHub" link.
client/src/bcd.scss
Outdated
} | ||
|
||
.bc-table .bc-platforms .bc-platform-desktop:before { | ||
background-image: url("/bcd/platforms/desktop.svg"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
background-image: url("/bcd/platforms/desktop.svg"); | |
background-image: url(./icons/bcd/platforms/desktop.svg); |
And to make this work you need to...
cd client
mkdir -p src/icons/bcd/platforms
git mv public/bcd/platforms/* src/icon/bcd/platforms/
(you can of course just mkdir src/icons ** git mv public/bcd src/icons/
or whatever it is)
Now, when you compile this, CRA comes with some fancy webpack SCSS sorcery that turns the CSS into...:
.bc-platform-desktop:before{background-image:url(/static/media/desktop.d6def92f.svg);
and it generates the file build/static/media/desktop.d6def92f.svg
All in all, it means we can confidently set a max cache-control
header on all URLs like ^/static/
plus if we ever change desktop.svg
it'll invalidate any browser caches immediately.
This applies to the /bcd/general/external-link.svg
mentioned above too.
client/src/bcd.scss
Outdated
.active.bc-supports-no .ic-history:before, td[tabindex].bc-supports-no:focus .ic-history:before, td[tabindex].bc-supports-no:hover .ic-history:before, | ||
.active.bc-supports-yes .ic-history:before, td[tabindex].bc-supports-yes:focus .ic-history:before, td[tabindex].bc-supports-yes:hover .ic-history:before, | ||
{ | ||
background-image: url("data:image/svg+xml;base64,PHN2ZyBzdHlsZT0iZmlsbDogI2Y4ZTFlMTsiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyIgd2lkdGg9IjE2IiBoZWlnaHQ9IjI4IiB2aWV3Qm94PSIwIDAgMTYgMjgiPjxwYXRoIGQ9Ik0xNiAxMWEuOTkuOTkgMCAwIDEtLjI5Ny43MDNsLTcgN0M4LjUxNiAxOC44OSA4LjI2NSAxOSA4IDE5cy0uNTE2LS4xMDktLjcwMy0uMjk3bC03LTdBLjk5Ni45OTYgMCAwIDEgMCAxMWMwLS41NDcuNDUzLTEgMS0xaDE0Yy41NDcgMCAxIC40NTMgMSAxeiIvPjwvc3ZnPg=="); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you download the and convert it into a real .svg
file. base64 data URIs should be avoided like the plague. :)
client/src/bcd.scss
Outdated
} | ||
|
||
.ic-deprecated:before { | ||
background-image: url("/bcd/emojis/thumbs-down.svg"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a feeling we're going to need to reuse some of these icons. E.g. See the side-bar on https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set
"thumbs-down.svg" doesn't feel very "BCD" specific.
If you think it's smart, perhaps avoid the "bcd" prefix for all of these cute little svgs unless there's some select few that are very distinct/unique to BCD.
client/src/index.scss
Outdated
@@ -1,2 +1,3 @@ | |||
@import "water.css/light"; | |||
@import "mdn"; | |||
@import "bcd" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this from index.scss
and instead, in ingredients/browser-compatibility-table/index.js
add...
import "./bcd.scss";
somewhere after all the other imports. You'll have to move the file bcd.scss
into the folder ingredients/browser-compatibility-table/
but rather if you do that I guess all the references to .svgs will need to become something like url(../../../icons/platforms/desktop.svg)
.
That might look ugly but it means apart from some general icons, the whole folder is self-contained. Perhaps in 2021 we decide to drop this whole idea of BCD, then we can just delete the whole folder plus the import in document.js
and be done.
|
||
return ( | ||
<VersionBlock | ||
key={index} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key
attributes should only be set on iterated things where we're asking React to render an array.
If BrowserSupportDetail
is used in a loop, that's where you set the index. E.g.
render() {
const { allDetails } = this.props;
return (
<div className="browser-support-details">
{allDetails.map(detail => <BrowserSupportDetail key={detail.someUniqueThing} browser={detail.browser} .../>)}
</div>
)
}
if (!support || (!support.notes && !support.flags && !support.prefix && !Array.isArray(support))) { | ||
return ( | ||
<VersionBlock | ||
key={index} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See note below about use of key=
if (displayBlock) { | ||
note.push( | ||
<VersionBlock | ||
key={`block-${indexNote.index}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key={`block-${indexNote.index}`} | |
key="block" |
It just needs to be different from the other React Node elements added to the note
array here in this context.
} | ||
if (displayNote) { | ||
note.push( | ||
<BrowserSupportNoteContent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a bit overkill to have this function at all. You can just go ahead and do like you do down on line 29 here in this same file.
Thank you for the review again, i believe all of the above should be resolved.
No worries, it's getting there 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some new nit pickeries.
client/src/document.js
Outdated
sections.push(<BrowserCompatibilityTable key="browser_compatibility_table" data={document.browser_compatibility}/>); | ||
} else { | ||
sections.push(<><br/><br/><em>This reference page does not have Browser Compatibility.</em></>); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note-to-self; how we're including the BCD table is going to change significantly. These above lines here don't matter.
|
||
return ( | ||
<VersionBlock | ||
key={index} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key={index} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure i removed that in the last commit. 👍
hasNotes: false, | ||
legendSet: false | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole constructor can be simplified to just:
state = {
currentNoteId: null,
hasDeprecation: false,
hasExperimental: false,
hasNonStandard: false,
hasFlag: false,
hasPrefix: false,
hasNotes: false,
legendSet: false
}
} | ||
|
||
gatherPlatformsAndBrowsers(document, category) { | ||
let platforms = ["desktop", "mobile"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a nitpick but can you use const
instead? It what's generally the convention in this project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would've but this stopped me from doing that.
if (category === "webextensions") {
displayBrowsers = [...BROWSERS["webextensions-desktop"], ...BROWSERS["webextensions-mobile"]];
platforms = ["webextensions-desktop", "webextensions-mobile"];
}
Which I took from https://github.com/mdn/kumascript/blob/master/macros/Compat.ejs#L64
I was a little confused by the complete rebuild of the array, but I'm not familiar enough to know the reasoning behind it.
this.setState({ currentNoteId: null }); | ||
} else { | ||
this.setState({ currentNoteId: currentNoteId }); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the above is a bit cryptic. The condition, can it not be expressed as:
onNotesClick = noteId => {
this.setState({currentNoteId: noteId === this.state.currentNoteId ? null : noteId});
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaner, I like it 👍
</tbody> | ||
</table> | ||
<Legend | ||
key="bc-legend" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key="bc-legend" |
<span className="bc-supports-yes bc-supports"> | ||
<abbr className="bc-level bc-level-yes only-icon" title="Full support"> | ||
<span>Full support</span> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be done with SCSS instead?
return ( | ||
<tr className="bc-platforms"> | ||
<td /> | ||
{platforms.map((platform, i) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{platforms.map((platform, i) => ( | |
{platforms.map(platform => ( |
<td /> | ||
{platforms.map((platform, i) => ( | ||
<th key={platform} className={`bc-platform-${platform}`} colSpan={Object.keys(browsers[platform]).length}> | ||
<span>{platform.charAt(0).toUpperCase() + platform.slice(1)}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option would be to
tr.bc-platforms th span {text-transform: capitalize;}
As I go through it I can't find anything that isn't a nit-pick or can't wait. What matters is forward momentum at this point. Well, functionality matters most, I guess. Can @Elchi3 @ddbeck or @wbamberg please check out this branch and run it locally with the pages that exist and say if it's working as expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I would love to hear from the more-BCD-experts to sanity check that it actually works. I'm so biased towards just landing this that I don't actually look at the functionality as a whole.
My approval, but merging ideally hinged on the MDN Content peeps taking a look.
I'm not sure if I mentioned this at one point or not, but I used the HTML elements to test with this. Are there other pages I am able to test with? If they become available at a later date I'll test them and make sure everything still works as intended. |
The issue was that the |
OK, I've only checked out the local preview and I haven't examined the code here at all. I'm offering my input as someone who looks at a lot of compat tables; please disregard if this is unhelpful. I have no doubt this was a lot of work. Clearly, it's 99% identical to the regular MDN BCD tables. 👍 Great job, @joedarc, and thank you. In content terms, the presentation of the individual cells of data seems fine. That said, I did spot a couple of oddities. First, the browser labels seem off (e.g., "IE" instead of "Internet Explorer" or "Chrome Android" instead of "Chrome for Android" and several other discrepancies). Second, it looks the BCD data is out of date. Strictly speaking, that last part might be stumptown-content's fault, but the reason I noticed is that there's Edge Mobile data. There's no reason to have code to render the Visually, it's very close to the regular MDN tables. I did notice a couple tiny visual surprises, though. The animation to expand a set of notes is a bit different (the triangle does a little spin, while the content snaps open immediately, where on the regular MDN site, the triangle snaps inverted while the content area slides open). The labels for the browsers don't seem to always land on the same line; see this screenshot for an example (WebView, from the For what it's worth, I think it's important to get the labels right (I think folks from some of the browser vendors bristle at these things), but the visual oddities can be fixed in follow up PRs, if y'all are keen to merge sooner rather than later. |
Is that the browser names? I.e. making it "Internet Explorer" instead of "IE"? The reason the data is out of date is because of an interesting problem. The git submodule link between stumptown-renderer and stumptown-content is out of date. I.e. renderer is pointing to an old version of content. Today, the reason for that is because of the rather big change to structure and this PR hopes to fix it.
That's many slow steps. The simplest and most powerful solution is to "reverse" the git submodule linkage. Instead of Perhaps I'm off on a tangent, but one of the things that sucks about Kuma is the long and akward delay between a BCD release and the Kuma Wiki updating tables with BCD data. I'm eager to hear from @joedarc if he can take a look at the browser name labels. |
Yes, that's correct.
I definitely subscribe to your concerns about the links between BCD, stumptown-content, and the renderer (and the existing delays with Kuma and BCD). I've been thinking about this a bit too. Plus, I'm not wild about submodules. They're sort of strange and surprising in a lot of ways (which, for Git, is saying something) and I worry contributor confusion. I've thought about a few alternatives. Maybe a lightweight service could always provide an always up-to-date source of data to stumptown builds. Or we maybe we could use GitHub Actions (in November) to dispatch events from BCD to stumptown-content, when there's a new release (at which point, stumptown could auto-update to the latest BCD). Might be worth opening an issue to discuss possible solutions, or to put it on the agenda to discuss in London. |
Good catch @ddbeck, don't know how that happened.
I was wondering why it wasn't in the BCD tables on prod when the data was there for it somewhat, I removed it from the initialization though. |
@joedarc Would you mind doing a quick rebase on this. It just needs to work the simplest way possible. I'm eager to look into lazy-loading your new wonderful React component once this hard work goes into |
b9eb527
to
3ed6eb0
Compare
Addressed review comments Addressed more review comments. Revert html-react-parser changes Addressed more review comments. Addressed further review comments. Better styling for code tags. Fixed browser names, removed edge_mobile.
@peterbe rebased, squashed, and signed. 👍 |
HURRAY AND THANK YOU TO @joedarc !!! |
This is a first pass at rendering the BCD tables using React Components. I made the main component
BrowserCompatibilityTable
a class and set the data forbrowser_compatibility
into state. Down the road if you want to pull that data from an API within this component, it'll be as simple as making the call in thecomponentDidMount
and setting state.Massaging the data to render HTML markup that is similar to the existing site was a bit of a struggle and there might be a better way to present that data from the API. For example, the data schema seems really inconsistent (sometimes things are objects, sometimes they're arrays, etc.)
The CSS was taken from current BCD tables, I made a few adjustments for responsiveness.
I'm unsure if there is a way to
build-json
for the other types of content, but i tested this against all of the html elements and it matched up pretty well. Let me know if you have any suggestions or questions regarding anything.