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

Image Component updated to v1 API #280

Merged
merged 1 commit into from
Aug 13, 2016

Conversation

jhchill666
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Jun 24, 2016

Current coverage is 93.85% (diff: 100%)

Merging #280 into master will increase coverage by 0.10%

@@             master       #280   diff @@
==========================================
  Files            73         74     +1   
  Lines           944        960    +16   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            885        901    +16   
  Misses           59         59          
  Partials          0          0          

Powered by Codecov. Last update 5534c7e...f011ce2

@levithomason
Copy link
Member

Image API Proposal

Most of this API is straight forward. However, the 3 variations of markup that need to be considered:

<img src="hi.png" class="ui image" />
<div class="ui image">
  <img src="hi.png" />
</div>
<a href="google.com" class="ui image">
  <img src="hi.png" />
</a>

There are some cases (comments, items, maybe more) where the image must be wrapped in order to work correctly. Heads up. This scratches the surface of a larger design question. How do you deal with internal component hierarchy?

We've had many discussions and proposals for dealing with component hierarchy. None are 100% ideal so far. Once we get to a component that has much more complicated internal hierarchy, we'll cover those in more detail. I would love to get some of your thoughts.

For now, I think we should just focus on the the 3rd one (link) and tackle the toggling of the div wrapped markup later.

link

The link case is special. The image must be wrapped in an a tag. The SUI classes then go on the wrapping a tag instead of the img. The src, alt, width, and height should still be applied to the img, of course.

Here's a proposal, would like to get some feedback:

<Image />                             // starting point for comparison
<Image link />                        // renders empty `a` tag
<Image href='goo.gl' hidden />        // `href` does the same thing as `link`
<Image link href='goo.gl' hidden />   // showing optional `link` with `href`
<img class="ui image" />

<a>
  <img class="ui image" />
</a>

<a href='goo.gl'>
  <img class="ui hidden image"/>
</a>

<a href='goo.gl'>
  <img class="ui hidden image"/>
</a>

hidden

useKeyOnly()

<Image hidden />
<img class="ui hidden image" />

disabled

useKeyOnly()

<Image disabled />
<img class="ui disabled image" />

avatar

useKeyOnly()

<Image avatar />
<img class="ui avatar image" />

bordered

useKeyOnly()

<Image bordered />
<img class="ui bordered image" />

fluid

useKeyOnly()

<Image fluid />
<img class="ui fluid image" />

rounded

shape (use prop value)

<Image shape='rounded' />
<img class="ui rounded image" />

circular

shape (use prop value)

<Image shape='circular' />
<img class="ui circular image" />

aligned

useAlignedProp()

<Image aligned='top' />
<Image aligned='middle' />
<Image aligned='bottom' />
<img class="ui top aligned image" />
<img class="ui middle aligned image" />
<img class="ui bottom aligned image" />

centered

useKeyOnly()

<Image centered />
<img class="ui centered image" />

spaced

useKeyOrValueAndKey()

<Image spaced />
<Image spaced='left' />
<Image spaced='right' />
<img class="ui spaced image" />
<img class="ui left spaced image" />
<img class="ui right spaced image" />

floated

useValueAndKey()

<Image floated='left' />
<Image floated='right' />
<img class="ui left floated image" />
<img class="ui right floated image" />

size

size (use prop value)

<Image size='mini' />
<Image size='tiny' />
<Image size='small' />
<Image size='medium' />
<Image size='large' />
<Image size='big' />
<Image size='huge' />
<Image size='massive' />
<img class="ui mini image" />
<img class="ui tiny image" />
<img class="ui small image" />
<img class="ui medium image" />
<img class="ui large image" />
<img class="ui big image" />
<img class="ui huge image" />
<img class="ui massive image" />

} = props

const classes = cx(
'sd-image', 'ui',
Copy link
Member

Choose a reason for hiding this comment

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

Heads up, I've finished removing all sd-* classes, see #301. Rebase to master to get the update. The test for the sd-* class has been inverted to fail if any sd- class exists.

Let's remove all sd-* classes from all PRs. 👍

@levithomason levithomason modified the milestone: v1.0 Jun 30, 2016
jhchill666 added a commit to jhchill666/stardust that referenced this pull request Jul 5, 2016
- Added all examples from Semantic docs
- Added tests for Image and ImageGroup
- Added ImageGroup class for grouping images
@levithomason
Copy link
Member

@jamiehill Heads up, I'm headed out for vacation this week. I'll catch you next week sometime. Cheers!

@levithomason
Copy link
Member

levithomason commented Aug 11, 2016

Note the comments in #383, modal updates. It relies on this Image PR being merged.

SUI supports two types of Image markup. This Image update PR should provide a way to configure which img markup variation is used:

<div class="ui image">
  <img />
</div>

<img class="ui image" />

The first example is required for Modal image content and others. Whereas, the second example is required for List images and others. Perhaps we allow:

<Image wrapped />

<Image />

Which would render the above HTML accordingly.

@levithomason
Copy link
Member

Heads up, I've begun working on this branch and will push soon.

@levithomason
Copy link
Member

This required refactoring some utils to resolve circular references, I've started that here https://github.com/TechnologyAdvice/stardust/pull/388.

@levithomason
Copy link
Member

I added docs and finished all the features. I also rebased to the latest master which includes the util refactors.

Will probably merge soon.

@levithomason levithomason merged commit f26392d into Semantic-Org:master Aug 13, 2016
@levithomason levithomason deleted the feature/image branch August 13, 2016 11:43
layershifter pushed a commit that referenced this pull request Aug 14, 2016
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