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

Data tab applab instructions #15381

Merged
merged 5 commits into from May 26, 2017
Merged

Data tab applab instructions #15381

merged 5 commits into from May 26, 2017

Conversation

epeach
Copy link

@epeach epeach commented May 25, 2017

Axosoft
Added explanatory text and captions to the data tab in applab.

@epeach epeach requested a review from davidsbailey May 25, 2017 17:49
Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Hi Erin,

It looks like you're off to a great start! I've got a couple comments for you below. Let's go over them together once you've had a chance to read through them.

@@ -0,0 +1,13 @@
@no_mobile
@dashboard_db_access
Copy link
Member

Choose a reason for hiding this comment

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

this test shouldn't need dashboard db access. please try removing this line, and then confirming that it passes on CircleCI or by running the test locally (I can help you with that).

@@ -61,6 +61,9 @@ const DataOverview = React.createClass({
<div id="dataOverview" style={{display: visible ? 'block' : 'none'}}>
<h4>Data</h4>

<h5>{msg.dataTabExplanation()} <a id="dataBlocksLink" href="https://code.org/applab/docs/tabledatastorage"><strong>{msg.dataBlocks()}</strong></a> {msg.withinApp()}</h5>
<br/>
Copy link
Member

Choose a reason for hiding this comment

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

unfortunately, because different languages have different grammar, this isn't an i18n (internationalization) friendly way to construct a string. Fortunately, our translation system has the ability to include placeholders in a string. Here is a simple example of the completedWithoutRecommendedBlock ui string using a placeholder:

Ideally you would do something like the following:

  1. define a dataBlocksLink variable to save the <a id="dataBlocksLink" href="https://code.org/applab/docs/tabledatastorage"><strong>{msg.dataBlocks()}</strong> snippet from your above code that renders msg.dataBlocks into a string
  2. create a single, longer ui string containing your dataTabExplanation, then a placeholder for the dataBlocksLink, then the contents of your withinApp string
  3. inject your dataBlocksLink in as the placeholder

@epeach
Copy link
Author

epeach commented May 25, 2017

@davidsbailey PTAL. Thanks!

@davidsbailey
Copy link
Member

Nice progress Erin. Could you please include a screenshot of how this is looking?

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

The code looks good to me, as long as Poorva or whoever requested this is happy with how it looks on the screen :)

@epeach epeach merged commit f88f25b into staging May 26, 2017
@epeach epeach deleted the data-tab-applab-instructions branch May 30, 2017 15:47
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