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

Manage Students - convert explanatory text to react #22466

Merged
merged 14 commits into from May 17, 2018

Conversation

caleybrock
Copy link
Contributor

@caleybrock caleybrock commented May 16, 2018

Summary

As I was refactoring, I noticed we had 3 react dom mounting points for the manage students table, which seemed unnecessary. One for oauth, one for the table itself, and one for the login type button.

This PR adds the login type button to the main table rendering, and also builds out the rest of the explanatory text below the table in react (and deletes the angular/haml version). The text felt disorganized and hard to read, so I made section headings to be more descriptive of the information. Screenshots show the before and after differences.

Getting a little tripped up with links and strings, and would appreciate advice.

These are UI changes, so I'll also need approval from @poorvasingal

Screenshots

Email sections

Before

screen shot 2018-05-16 at 11 18 46 pm

After

screen shot 2018-05-16 at 11 16 58 pm

Picture and word sections

Before

screen shot 2018-05-16 at 11 19 02 pm

After

screen shot 2018-05-16 at 11 15 49 pm

<p>
{i18n.joinSectionAsk()}
<a target="_blank" href={`${window.location.origin}/join/${sectionCode}`}>
{`http://localhost-studio.code.org:3000/join/${sectionCode}`}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm struggling to figure out how to make these URLs dynamic based on environment, since there are both studio and code.org urls and this is from apps in pegasus. Any ideas are welcome.

Copy link
Member

Choose a reason for hiding this comment

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

Generally speaking, you'll want to find the right method in deployment.rb (possibly dashboard_hostname), pass it into your js entry point in the data tag on the script element, and then pass it through props into your react component. I'll try to dig up an example.

Copy link
Member

Choose a reason for hiding this comment

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

looks like studioUrlPrefix is already available here:

const studioUrlPrefix = scriptData.studiourlprefix;

@@ -573,6 +573,8 @@
"instructions": "Instructions",
"joinASection": "Join a section",
"joinSection": "Join section",
"joinSectionExplination": "Ask your students to join your section by going to this link and entering the section code (above): ",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what a better way to localize these strings would be - I've just made it so the links are at the end and generated in JS.

Copy link
Contributor

Choose a reason for hiding this comment

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

We've run into similar problems with links in the context of strings, and this is consistent with solutions I've seen in the past.

// Because 'temporary' students are included in $scope.section.students
// before we reach this save() action, if _all_ students are new
// students then we had zero saved students to begin with.
// TODO: Once everything is React this should become unnecessary.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing these TODOs was the main goal of this re-write PR.

@caleybrock
Copy link
Contributor Author

Note to self: change base back to staging once parent PR is merged.

@caleybrock caleybrock changed the title Manage Students - convert explanatory text - WIP Manage Students - convert explanatory text to react May 17, 2018
{(loginType === SectionLoginType.word || loginType === SectionLoginType.picture) &&
<div>
<p>
{i18n.sectionCode() + ': ' + sectionCode}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason i18n string sometimes include the : and sometimes you add them in line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mainly because I wanted to use this string that already existed.

@caleybrock
Copy link
Contributor Author

I'm going to create a way to make relative URLs in pegasus in a separate PR and then come back and update this page. For now, I've hardcoded the production urls into the UI.

@@ -573,6 +573,8 @@
"instructions": "Instructions",
"joinASection": "Join a section",
"joinSection": "Join section",
"joinSectionExplination": "Ask your students to join your section by going to this link and entering the section code (above): ",
Copy link
Member

Choose a reason for hiding this comment

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

spelling: "Explanation", here and in a few other places

@Erin007
Copy link
Contributor

Erin007 commented May 17, 2018

+1 to finding a good way to get relative links in pegasus. I had the same problem earlier:
screen shot 2018-05-17 at 12 54 19 pm
and ending up working around by getting the whole path I wanted from the ruby side in https://github.com/code-dot-org/code-dot-org/pull/21807/files 😀

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.

Nice to see so much angular going away! LGTM after fixing studio urls, please let me know if you want me to take another look.

@caleybrock
Copy link
Contributor Author

I got URLs working via Dave's suggestion. Still not a general solution, but it works for this item. Thanks for the help!

@caleybrock caleybrock merged commit 722bf01 into extract-set-section May 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants