Skip to content

Review PR for Ned and Xavier - #18

Closed
martynjames wants to merge 63 commits into
nullfrom
master
Closed

Review PR for Ned and Xavier#18
martynjames wants to merge 63 commits into
nullfrom
master

Conversation

@martynjames

Copy link
Copy Markdown

Complete review PR for xblock-google-drive

marjev and others added 30 commits November 11, 2014 10:13
Looking great - a couple of things that I've discovered when testing, I'm going to log some defects to help us track addressing them.
UI string review for google docs and calendar
no longer constrain height to 100% of parent - width constraint is sufficient
Documention on css changes and validation
…d code, and slight reordering for ease of reading
@martynjames martynjames changed the title Review PR for Matt Review PR for Ned Feb 13, 2015
Comment thread README.md 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.

A minor nit, but it would be great to be in the habit of writing README.rst files instead of README.md files.

@marjev

marjev commented Feb 19, 2015

Copy link
Copy Markdown
Contributor

@smagoun @nedbat @antoviaque

This PR is left for review purposes. Closed PR-21 and PR-22 and implemented changes that were suggested in the comments within those PRs.

@marjev marjev changed the title Review PR for Ned Review PR for Ned and Xavier Feb 19, 2015
@antoviaque

Copy link
Copy Markdown

Posted a couple of answers/nits in answer to your comments on #23 but once addressed the changes made LGTM 👍

@marjev

marjev commented Feb 20, 2015

Copy link
Copy Markdown
Contributor
  1. Changed README.md to README.rst
  2. Values commonly used in tests are now contained in constants
  3. Changed description text for alternative text field

@nedbat

nedbat commented Feb 23, 2015

Copy link
Copy Markdown
Contributor

👍

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.

9 participants