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

Implement skeleton for Test Details Views #44

Merged
merged 4 commits into from Oct 2, 2019

Conversation

sarathms
Copy link
Contributor

Implements 1 out of 7 views listed in #3

@sarathms sarathms added this to In Progress in DO NOT USE (was: OONI Probe Desktop) via automation Sep 26, 2019
@sarathms
Copy link
Contributor Author

As of now, we have a working structure. Specific data like the content for Hero (and an alternative collapsed Hero), test details come from its own /nettests/websites/WebConnectivity.js file. This file also holds metadata about that test.
Common content will be generated and rendered in MeasurementContainer itself.

Also added a collection called tests in nettestes/index.js to map metadata for all test. So far we only export the translated name of the test. More can be added.

@hellais Please review and point out holes in this approach.

image

image

@hellais
Copy link
Member

hellais commented Sep 30, 2019

As of now, we have a working structure. Specific data like the content for Hero (and an alternative collapsed Hero), test details come from its own /nettests/websites/WebConnectivity.js file. This file also holds metadata about that test.
Common content will be generated and rendered in MeasurementContainer itself.

Also added a collection called tests in nettestes/index.js to map metadata for all test. So far we > only export the translated name of the test. More can be added.

I took a look at the code as well and this approach looks solid to me 👍

@hellais
Copy link
Member

hellais commented Sep 30, 2019

Is it useful if I make a mockup of how the views in collapsed vs expanded form shall look like?

Copy link
Member

@hellais hellais left a comment

Choose a reason for hiding this comment

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

Looks good to me

@sarathms sarathms changed the title Implement Test Details View for WebConnectivity measurements Implement skeleton for Test Details Views Oct 2, 2019
@bassosimone
Copy link
Member

I suggest to merge this PR and then follow up with others! :shipit:

@sarathms
Copy link
Contributor Author

sarathms commented Oct 2, 2019

I had made some changes to the approved model because I immediately found a problem with it. I will send them for review here in the next few minutes.

@sarathms
Copy link
Contributor Author

sarathms commented Oct 2, 2019

The latest commit helps compose a few common UI elements in the Hero (like Test Name, Date, Network) in the Container component, instead of all the test specific components. This also prevents having to pass in the <BackButton> component deeper into the hierarchy.

image

Copy link
Member

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

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

Looks good! I've added some quick suggestions I'd like to be applied before merging!

🐳

@@ -89,6 +91,24 @@ export const testGroups = {
}
}

export const tests = {
web_connectivity,
Copy link
Member

Choose a reason for hiding this comment

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

This is syntactic sugar for web_connectivity: web_connectivity, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct.

return (
<div>
{render({
heroBG: 'green8', // TODO: Determine color based on presence of anomaly
Copy link
Member

Choose a reason for hiding this comment

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

Suggested // TODO(sarathms): ... so we know who's owning the TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

whatsapp: web_connectivity,
ndt: web_connectivity,
dash: web_connectivity,
vanilla_tor: web_connectivity,
Copy link
Member

Choose a reason for hiding this comment

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

If your goal is to later replace each of them, please add a TODO(sarathms): ... comment to indicate that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added TODO

@@ -97,32 +99,144 @@ const HeaderContent = styled(Box)`
-webkit-app-region: drag;
`

const detailsMap = {
web_connectivity: WebConnectivity,
facebook_messenger: FacebookMessengerDetails
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that more will come later? If so, please add a // TODO(sarathms): ... comment to say that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@sarathms sarathms merged commit 63bf274 into master Oct 2, 2019
DO NOT USE (was: OONI Probe Desktop) automation moved this from In Progress to Done Oct 2, 2019
@sarathms sarathms deleted the ux/websites-result-details branch October 3, 2019 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
3 participants