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

Feature GH-981: Add HOC banner component #1020

Closed
wants to merge 3 commits into from

Conversation

mewtaylor
Copy link
Contributor

Implements #981.

A couple things to note about this from the specs:

  • modals disappear for tablet and below (because the tiles in the banners disappear)
  • The teacher banner, for teachers, will appear above the hoc banner

Test Cases

  • Banner appears for logged in and logged out users
  • Banner appears below teacher banner for teachers
  • Tiles in banner show modal when the "see guides" part is clicked
  • Modals disappear below our desktop breakpoint
  • Tiles disappear below our desktop breakpoint

we need it to be different for the hoc banner, so make it need a class name in order to be styled correctly
Having it in the nav was causing issues with positioning the banner, as evidenced by the teacher banner’s `-20px` setting
}
};

return [

This comment was marked as abuse.

This comment was marked as abuse.

Copy link
Contributor

@rschamp rschamp left a comment

Choose a reason for hiding this comment

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

My comment is more of a yellow than a red, so I think this looks good to go overall.


var tileObjects = {
flyTile: {
title: formatMessage({id: tiles[1].title}),

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@mewtaylor
Copy link
Contributor Author

@rschamp @chrisgarrity – for Nov 21 hotfix, we either need to not merge any December things into develop before Monday, or maybe create a separate hotfix branch (which would need to get PRs for the release branch too). Thoughts?

@rschamp
Copy link
Contributor

rschamp commented Nov 16, 2016

@mewtaylor I think a hotfix branch would make the most sense. Can you explain what you mean about getting PRs for the release branch?

@mewtaylor
Copy link
Contributor Author

@rschamp I mean that if there's a PR for a bugfix found during QA that needs to go against the release branch, I'll need to also open a PR against the hotfix branch that we create – unless we just don't merge this until after the release.

@chrisgarrity
Copy link
Contributor

@mewtaylor @rschamp A hotfix branch makes sense to me, but I might wait a couple of days any any case to confirm the order of the tiles on the HOC banner.

@mewtaylor
Copy link
Contributor Author

Going to re-open against release/hoc-nov-21

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.

3 participants