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
Revive diversity survey for 2018 #22363
Conversation
The follow up google survey currently links to this, which is closed. I'm following up to re-open and modify as needed before I merge this in. We may remove the button and follow up survey altogether. |
@@ -111,7 +111,7 @@ export default class TeacherHomepage extends Component { | |||
|
|||
componentDidMount() { | |||
// The component used here is implemented in legacy HAML/CSS rather than React. | |||
$('#terms_reminder').appendTo(ReactDOM.findDOMNode(this.refs.termsReminder)).show(); |
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 tried renaming the ref to more accurately describe the things it shows, but I couldn't rename #terms_reminder
to #teacher_reminders
. When I renamed it (on both this .jsx page and in the _homepage.html.haml page), the layout would go wonky and the reminders would be placed on top of the banner. I couldn't find any css styles attached to this ID so I'm not sure why only #terms_reminder
works to show the views below the banner.
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'm seeing the wonkiness on this PR as well:
The trick is that we don't want the version in the HAML to be visible; it should be wrapped in something marked display: none
. Then, the HTML is copied from there and appended to the React DOM, in this appendTo
line. Once we're showing it in the React DOM, then it should appear below the React that contains the "My Dashboard" heading and provides the appropriate spacing.
Note that #terms_reminder
was the parent div for a variety of components, not just this one, so it was responsible for making several things available in React. It's likely that this could get the display: none
treatment so that none of the components under it, if they were being rendered, would be shown, assuming that all should now be in the React DOM.
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.
@Erin007 since you added in the terms reminder here, can you comment on what I'm missing here to fix my PR? :) I also don't understand why I would see my layout just fine but Brendan would see it wonky for the same PR...
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.
looking...
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.
So I pulled the latest changes and it looks okay now. The survey being shown is part of the React DOM which is how it should work.
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 was also able to do the rename just fine. You have to make sure apps rebuilds successfully for that rename to work on the React side.
- if current_user | ||
- if current_user.teacher? | ||
#terms_reminder | ||
- unless current_user.accepted_latest_terms? | ||
= render template: 'api/accept_terms_reminder' | ||
= render partial: 'layouts/terms_interstitial' | ||
- if show_nps_survey? SurveyResult::NET_PROMOTER_SCORE_2017 |
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 decided to move it here because the index.html.haml
page contained the modal pop-ups like asking for school and accepting the terms of service. I thought this would be clearer that here are all the in-page reminders that show above the classroom sections.
Tested that the results are saved as
|
Confirmed that doing an apps build after renaming Now checking Circle tests... |
Bumped counts in tests so waiting for Circle CI to re-run. Ready for review! |
I restarted your Circle build, which should resolve the issue; but feel free to merge whenever you're ready. |
Muchas gracias! |
Reverts #14543
With both terms + NPS reminders (NPS is currently deactivated so we can restyle when we reactivate)
With both terms + diversity reminders
Diversity survey alone part 1/2
Diversity survey alone part 2/2