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

Remove HOC 2017 specific content on homepage, /learn & teacher home #19522

Merged
merged 4 commits into from Dec 14, 2017

Conversation

breville
Copy link
Member

@breville breville commented Dec 6, 2017

before

home

screenshot 2017-12-07 14 27 39

/learn

screenshot 2017-12-07 14 28 03

teacher home

screenshot 2017-12-07 14 27 25

after

home

screenshot 2017-12-08 14 01 15

/learn

screenshot 2017-12-07 14 28 58

teacher home

screenshot 2017-12-08 14 15 39

@@ -42,7 +39,7 @@ export default class TeacherHomepage extends Component {

render() {
const { courses, topCourse, announcements, isRtl, queryStringOpen, joinedSections } = this.props;
const { canViewAdvancedTools, hocLaunch, isEnglish } = this.props;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't need isEnglish here, do you still need it in PropTypes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I deliberately left it there because it was plumbed through and seemed like it might be useful again in the future, but you're right that we're not using it right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference would be to remove these when they're no longer used. If we added this back in the future ideally it would be connected via Redux.

@@ -130,8 +62,6 @@
- elsif rack_env != :production && request.params["lock-hero"]
-# for UI tests just lock to the first hero image
- heroes_arranged = heroes[0, 1]
- elsif show_hoc_launch_layout
- heroes_arranged = hoc_launch_hero[hoc_launch_hero_variation]
- elsif show_census_layout
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea is that we don't know what the homepage takeover will be for next year so we should gut anything related to hoc? Just confirming there isn't anything reusable.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was my thinking. I think we could generalise this technique going forward, but it should probably be done in conjunction with a rethink about the gsheet-driven approach for driving the homepage in general.

Copy link
Contributor

@Erin007 Erin007 left a comment

Choose a reason for hiding this comment

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

LGTM. Are we linking this PR to a doc for 2018 HoC? It would be a good reference to have handy if we want to do something similar for next year.

@tanyaparker
Copy link
Contributor

tanyaparker commented Dec 6, 2017

Creating a 2018 hoc doc.

📷 ? Or is this just code cleanup and actual changes of the homepage will come later?

@breville
Copy link
Member Author

breville commented Dec 8, 2017

📷 added

@tanyaparker
Copy link
Contributor

tanyaparker commented Dec 8, 2017

After teacher home image is a repeat?

@breville
Copy link
Member Author

breville commented Dec 8, 2017

Oops, fixed.

@breville breville merged commit 561a8a5 into staging Dec 14, 2017
@breville breville deleted the remove-special-ui-for-hoc-2017 branch December 14, 2017 09:21
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

4 participants