Skip to content

bug 796871 - new app for gameon - #404

Merged
craigcook merged 1 commit into
mozilla:devfrom
rossbruniges:gameon-page
Oct 9, 2012
Merged

bug 796871 - new app for gameon#404
craigcook merged 1 commit into
mozilla:devfrom
rossbruniges:gameon-page

Conversation

@rossbruniges

Copy link
Copy Markdown
Contributor

In addition noticed that the country select box we use for email sign-ups is now used in 3 different places, so removed duplication and create an external include file for this, and dropped it into a new app called 'foundation'

Comment thread apps/gameon/urls.py Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Check out mozorg.utils.page, it'll let you get rid of views.py completely.

@Osmose

Osmose commented Oct 8, 2012

Copy link
Copy Markdown

I'll leave the rest of the review to @craigcook since he's a frontend master.

@rossbruniges rossbruniges reopened this Oct 8, 2012
@rossbruniges

Copy link
Copy Markdown
Contributor Author

Yes - I closed it by accident. Good news, I didn't press the merge pull request. Open now for any further reviews.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add aria-required="true" here to support browsers/AT that doesn't support just required :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also No <label> element associated with the select element

@craigcook

Copy link
Copy Markdown
Contributor

Preferably this page should have a responsive layout unless there's a compelling reason for it not to. We're working to phase out all of the static width pages and all new pages on mozilla.org should use the responsive template.

I can help guide you through making it responsive (just entails referencing some different template files and setting up the media queries). The banner image will be the only tricky part but it can probably just scale.

@rossbruniges

Copy link
Copy Markdown
Contributor Author

@craigcook ah right, that isn't baked into the templates by default?

Show me what ones to use and I'll drop stuff in there - as you say, easy enough to sort out :)

@rossbruniges

Copy link
Copy Markdown
Contributor Author

@craigcook @icaaq thanks for looking over this - have updated the pull request fixing all the nits hopefully, and adding in the reponsive layout to boot!

Comment thread apps/gameon/templates/gameon/index.html Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No need for &nbsp; here, just an empty block will do.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll try again - but in the past I've found I do need to put in the non-breaking space...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

…y drop down box which is used on gameon, collusion and webmaker
craigcook added a commit that referenced this pull request Oct 9, 2012
bug 796871 - new app for gameon
@craigcook
craigcook merged commit cd7417e into mozilla:dev Oct 9, 2012
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.

4 participants