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

505/Tile Select Component #752

Merged
merged 60 commits into from Jul 31, 2020
Merged

505/Tile Select Component #752

merged 60 commits into from Jul 31, 2020

Conversation

eriklharper
Copy link
Contributor

image
image

…'s not what normal radio inputs do and we also need to set a value without it displaying for the new tile-select component
…render just a radio when a label isn't present
…yling from tile-select programmatically. Also adding hide-input property to implement the basic view
…checkbox when omitting the value attribute for convenience when using with calcite-tile-select
…ed standalone but embeddable for calcite-tile-select
…description text takes full horizontal space
…rid where it makes sense to and use absolute positioning where it makes sense
@macandcheese
Copy link
Contributor

@eriklharper want to sort out conflicts and we can get this in...

My only question is re: nomenclature of the wrapping component... should we use calcite-tile-group vs calcite-tile-select to better align with radio-button-group and radio-group cc @Esri/calcite-components

@eriklharper
Copy link
Contributor Author

@eriklharper want to sort out conflicts and we can get this in...

My only question is re: nomenclature of the wrapping component... should we use calcite-tile-group vs calcite-tile-select to better align with radio-button-group and radio-group cc @Esri/calcite-components

Word. Would love to get this in. Last little bit I almost have done is just the story file for calcite-tile-select. I'll push that up as soon as I can and we can merge this along with resolving the conflicts.

As far as the wrapping component name goes, I named the component calcite-tile-select at @bstifle 's request and since that component just renders a single tile with either a radio or checkbox (instead of a group of them) I think the name calcite-tile-select-group still fits since there's a separate calcite-tile component that just renders the tile UI with an optional href property for supporting linking it to somewhere. Right now the calcite-tile-select-group is such a light wrapper that what it does right now can be easily accomplished with just some basic flexbox styles, so I can see getting rid of it as well, but I thought including it might make using it nicer for people who want to make the boxes vertically stretch their borders to match others in the same row when the content length varies.

@macandcheese
Copy link
Contributor

That nomenclature makes sense - I think my only other feedback is we can probably lose the embed type for tile since it makes the component almost border on a layout helper and might be abused, IMO. This is looking great!

@bstifle
Copy link

bstifle commented Jul 30, 2020

@eriklharper yeah im not sure what the embed was for either. what was the reason for it?

@eriklharper
Copy link
Contributor Author

eriklharper commented Jul 30, 2020

@eriklharper yeah im not sure what the embed was for either. what was the reason for it?

I built calcite-tile as a separate component since you mentioned that it needed two use cases:

  1. A hyperlink
  2. A radio or checkbox

If we don't need to support use case 1 we would get rid of calcite-tile and move all that UI back into calcite-tile-select where I originally had it.

@macandcheese
Copy link
Contributor

No tile is definitely justified @eriklharper. Just referring to the "embed" style on the dev demo page for calcite-tile not being needed :)

@eriklharper
Copy link
Contributor Author

No tile is definitely justified @eriklharper. Just referring to the "embed" style on the dev demo page for calcite-tile not being needed :)

Totally gotcha now! I'll remove that from the demo, cause yeah that's just really a quasi-internal thing that's only used for calcite-tile-select.

@eriklharper eriklharper requested a review from a team as a code owner July 30, 2020 17:40
@eriklharper
Copy link
Contributor Author

Ok I'm all done now with the stories. Thanks @macandcheese for helping me fix my issue with storybook. Everything looks ready to go now.

@eriklharper eriklharper removed the request for review from a team July 30, 2020 19:03
:host([icon][heading]:not([description]):not([embed])) {
padding: unset;
}
.tile {
Copy link
Member

Choose a reason for hiding this comment

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

I think you could remove some of the nesting here. It doesn't really matter a ton because shadow dom makes css life much easier, but in theory you'd save some precious bytes.

.tile {
  display: grid;
  grid-template-columns: 1fr;
  grid-gap: $spacing;
}

.heading {
  @include font-size(0);
  color: var(--calcite-ui-text-1);
  font-weight: 500;
}

.large-visual {
  justify-items: center;
  min-height: 200px;
  .icon {
    align-self: self-end;
  }
  .heading {
    align-self: center;
  }
}

.description {
  @include font-size(-1);
  color: var(--calcite-ui-text-2);
}

@paulcpederson
Copy link
Member

So are all the indentation changes from prettier, or how did that happen? @jcfranco I think set up tooling to auto indent, but not sure how it's different or why those diffs are there.

Anyways, had a couple comments, but looks good, nice work @eriklharper !

@eriklharper
Copy link
Contributor Author

So are all the indentation changes from prettier, or how did that happen? @jcfranco I think set up tooling to auto indent, but not sure how it's different or why those diffs are there.

Anyways, had a couple comments, but looks good, nice work @eriklharper !

I was wondering the same thing. I wonder if I try to ditch all those changes (pull them from master) if they will regenerate. I'll try that.

@eriklharper eriklharper merged commit 0c5a8f5 into master Jul 31, 2020
@eriklharper eriklharper deleted the 505/radio-tile-component branch July 31, 2020 18:03
@bstifle
Copy link

bstifle commented Jul 31, 2020

Yay! Awesome work Erik!!!! 🇺🇸

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.

Radio Tiles / Tile Select
4 participants