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

Enabling the Tile Component to utilise the image component #23

Merged
merged 7 commits into from
May 29, 2015

Conversation

jodiedoubleday
Copy link
Contributor

What does this PR do?

  • Adjust the tile component to utilise the image component
  • Updates the docs
  • Adds some default styling for images
  • Spaces out the articles on docs better

What unit or integration tests does this PR have?

None

What selenium tests does this PR have?

None

How should a developer review this?

The best way to test this would be to console.log in the image component and make sure the log is shown when running the docs

How should this be manually tested?

npm install
grunt docs
Check that images are displayed on the docs

Any background context you want to provide?

  • The image component was produced but the tile needed to be updated.

What are the relevant tickets?

UXUI-250

What gif best describes how you feel about this work?

drunken monkey

Developer Definition of Done/Quality Checklist (for PR author to complete BEFORE code review):

  • I've checked there is appropriate unit, selenium and regression test coverage.
  • I've updated documentation with any changes to procedures.
  • I've tested this cross-browser/device for visual changes.
  • I've checked this work against the requirements of the Jira.
  • I've manually checked this work against any dependent systems.
  • I've added and updated translations for all supported languages.
  • I've informed those who need to know of any unforeseen impact of this work (database migrations, deployment procedure changes).
  • I've checked that there are no negative speed or performance impacts from my work.
  • I've added appropriate tracking.
  • I am ready for this to be code reviewed, merged and tested.

IP or Developer Review:

  • I’ve witnessed the work behaving as expected.
  • I’ve run all the tests locally and they pass.
  • +1 from me!

Software Engineer or Developer review:

  • I’ve witnessed the work behaving as expected.
  • I’ve run all the tests locally and they pass.
  • I’ve checked for appropriate test coverage.
  • I’ve checked for coding anti-patterns.
  • I've checked this work against the requirements of the Jira.
  • +1 from me!

Software Engineer or project guru review:

  • I’ve witnessed the work behaving as expected.
  • I’ve run all the tests locally and they pass.
  • I’ve checked for appropriate test coverage.
  • I’ve checked for coding anti-patterns.
  • I don’t believe this work introduces unnecessary technical debt.
  • +1 from me!

@SkylerAvery
Copy link

Taking IP/Dev

@@ -1,6 +1,6 @@
var image = {
src: 'http://brand.holidayextras.com/img/product-main.jpg',
alt: 'Two course meal'
alt: 'Tender sirloin steak with blue cheese dressing'

Choose a reason for hiding this comment

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

Should we change <UIToolkit.Tile image={image} title on line 7, to also equal this?

@matthewjtyas
Copy link

Taking SE/Dev

@matthewjtyas
Copy link

Looks good to me +1

@lukehansell
Copy link
Contributor

taking guru

@lukehansell
Copy link
Contributor

Can we start adding some real tests for these components now? Perhaps one that asserts the imageComponent is rendered and is in the correct place would be a good start?

@SkylerAvery
Copy link

👍

@jodiedoubleday
Copy link
Contributor Author

@lukehansell-hx There is a PR to move from JEST to Mocha/Chai so I don't want to write tests here which conflict. More tests will be written once we have moved.

@lukehansell
Copy link
Contributor

👍 from me...

can I ask what the change in test suite is for? I'm not overly fond of Jest, but I do like the stubbing functionality it gives us.

@jodiedoubleday
Copy link
Contributor Author

@lukehansell-hx we have moved for a few reasons..

  • We couldn't get coverage reports for Jest (JSX issues)
  • Jest doesn't support Node 12

jodiedoubleday pushed a commit that referenced this pull request May 29, 2015
Enabling the Tile Component to utilise the image component
@jodiedoubleday jodiedoubleday merged commit 30ef63e into master May 29, 2015
@jodiedoubleday jodiedoubleday deleted the UXUI-260 branch May 29, 2015 09:50
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