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.

Sign up for GitHub

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

Jump to bottom

feat: add container to render image as background behind children #70

Merged
merged 5 commits into from
Feb 25, 2019
Merged

feat: add container to render image as background behind children #70

merged 5 commits into from
Feb 25, 2019

Conversation

frederickfogerty
Copy link
Contributor

@frederickfogerty frederickfogerty commented Dec 27, 2018 •
edited by sherwinski
Loading

Description

This PR adds a new component which enables an image to be rendered as a background behind children components.

This PR has existed for a while in react-imgix, and is being ported over to this library.

New Feature

  • If this is a big feature with breaking changes, consider [opening an issue][issues] to discuss first. This is completely up to you, but please keep in mind that your PR might not be accepted.
  • Run unit tests to ensure all existing tests are still passing
  • Add new passing unit tests to cover the code introduced by your PR
  • Update the readme
  • Add some steps so we can test your cool new feature!
  • The PR title is in the conventional commit format: e.g. feat(<area>): added new way to do this cool thing #issue-number
  • Add your info to the contributors array in package.json!

Steps to Test

Review new tests.

jayeb
jayeb reviewed Jan 11, 2019
View reviewed changes
Copy link

@jayeb jayeb left a comment

Choose a reason for hiding this comment

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

One lil question, but otherwise this looks great.

addon/components/imgix-bg.js
);
const newDpr = toFixed(2, window.devicePixelRatio || 1);

set(this, '_width', newWidth);
Copy link

Choose a reason for hiding this comment

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

Will setting all three of these in sequence like this trigger three recomputes on the _src property, or is it debounced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've verified by testing on the example app that this only triggers one recompute 👍

jayeb
jayeb suggested changes Jan 16, 2019
View reviewed changes
addon/components/imgix-bg.js Outdated
@@ -93,6 +92,7 @@ export default Component.extend(ResizeAware, {
'fit',
'disableSrcSet',
function() {
console.log('compute src');
Copy link

Choose a reason for hiding this comment

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

This console.log was probably checked in by accident?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops... nothing to see here.

@frederickfogerty
Copy link
Contributor Author

NB: This should incorporate the changes brought in by #72 before being merged @sherwinski @jayeb

@sherwinski sherwinski force-pushed the fred/background-mode branch 2 times, most recently from da03e0f to 43f591e Compare February 22, 2019 19:18
@sherwinski
Copy link
Contributor

@jayeb
All changes from #72 should now be integrated with @frederickfogerty's development here. I also went through and cleaned up the commit history a bit.

jayeb
jayeb approved these changes Feb 22, 2019
View reviewed changes
Copy link

@jayeb jayeb left a comment

Choose a reason for hiding this comment

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

One little nit, but otherwise I think this is ready to go 👍.

tests/integration/components/imgix-background-test.js Outdated
});

module('default css class', function() {
test('it allows setting overriding the default class via config', async function(assert) {
Copy link

Choose a reason for hiding this comment

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

[nit] Lil typo here

jayeb
jayeb suggested changes Feb 25, 2019
View reviewed changes
README.md Outdated
@@ -225,6 +225,25 @@ module.exports = function(environment) {
```hbs
{{imgix-image path="/test.png" disableLibraryParam={true} }}
```
### imgix-bg

This component will render a `div` whose `background-image` is set to the passed in image path. Content can be added within the `imgix-bg` tags and the component will automatically resize to fit around it.
Copy link

Choose a reason for hiding this comment

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

Let's switch "passed in" to "given".

README.md Outdated
{{/imgix-bg}}
```

Which will generate html similar to the following:
Copy link

Choose a reason for hiding this comment

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

Can we say "This will generate..." instead?

README.md Outdated
Which will generate html similar to the following:

```html
<div id="ember226" alt="" style="background-image: url('https://my-social-network.com/users/1.png?fit=crop&w=1246&h=15&dpr=2&ixlib=ember-2.0.0');background-size: cover" class="imgix-bg ember-view">
Copy link

Choose a reason for hiding this comment

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

Let's distill this example a little bit by removing the id and alt attributes.

README.md Outdated
</div>
```

**Note:** `imgix-bg` will respect any global default parameters unless explicitly overriden
Copy link

Choose a reason for hiding this comment

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

[nit] This sentence is missing a period.

@sherwinski sherwinski merged commit 541f3d6 into master Feb 25, 2019
@sherwinski sherwinski deleted the fred/background-mode branch February 25, 2019 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jayeb jayeb jayeb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@frederickfogerty @sherwinski @jayeb