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

Add Material Design "Card" as widget.CardContainer #1262

Closed
wants to merge 14 commits into from

Conversation

andydotxyz
Copy link
Member

Description:

A basic card container with header, subheader, content and image - all optional.
We can add more to this later, such as the buttons (style challenges as they are depicted like hyperlink...?)

Deprecate Group in favour of CardContainer and use it in a few places where group was before.
Tests are just image comparison at this time as it's a pretty simple linear layout.

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.
  • Public APIs match existing style.
  • Any breaking changes have a deprecation path or have been discussed.

@andydotxyz
Copy link
Member Author

Current look is as below:

Screenshot 2020-08-26 at 11 13 04

widget/card.go Show resolved Hide resolved
widget/card.go Show resolved Hide resolved
widget/card.go Show resolved Hide resolved
func (c *CardContainer) SetTitle(text string) {
c.Title = text

c.Refresh()
Copy link
Member

Choose a reason for hiding this comment

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

It's not necessary to refresh the whole container, and depending on the contents this might be expensive... maybe the labels and image could be children of CardContainer rather than it's renderer.

Edit: the same applies to the other Set* methods below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, It's not that simple - if it were set to "" (or from "") then we do need a complete refresh.
Should that logic be in her to compare old and new values to do the different type of refreshes?

Copy link
Member

Choose a reason for hiding this comment

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

Considering that the content could potentially be very expensive to refresh, I think it makes sense to avoid propagating it down the line needlessly. In particular, I imagine a common use case for cards would be as a container for a custom widget -- downstream developers may not be as careful about avoiding spurious refreshes, so I think for the best developer experience we should do as much of the legwork as possible in that area.

@charlesdaniels
Copy link
Member

Two other things:

It looks like you have some changes in here that I thought were in other PRs? Maybe I haven't been keeping up, but I thought the list widgets was in #1235. Also it looked like there were some changes to vendor-ed deps. Maybe that's intentional, just thought it was worth pointing out.

IMO, the borders are too thin/light. Their contrast is so low they are completely invisible on my secondary monitor, a Dell 1908FP. Not exactly a new model, but recent enough that our widgets should be usable on it.

Copy link
Member

@stuartmscott stuartmscott left a comment

Choose a reason for hiding this comment

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

This PR looks to include unrelated changes from #1253

widget/card.go Show resolved Hide resolved
widget/card.go Show resolved Hide resolved
@andydotxyz
Copy link
Member Author

Closed in favour of #1263, will try to incorporate comments there, though questions here I will answer

@andydotxyz andydotxyz closed this Aug 26, 2020
@andydotxyz
Copy link
Member Author

This PR looks to include unrelated changes from #1253

Yes, sorry about that - the fixed PR is now #1263

@andydotxyz
Copy link
Member Author

IMO, the borders are too thin/light.

It's just going with the "1dp" elevation like the spec. Perhaps we need to adjust shadow renderer to give more definition?

@charlesdaniels
Copy link
Member

It's just going with the "1dp" elevation like the spec. Perhaps we need to adjust shadow renderer to give more definition?

Maybe this is a separate issue. But on one of my monitors the borders are completely invisible. It's an older monitor with crappy contrast, but it's not old enough I would be willing to say we shouldn't support it.

@andydotxyz
Copy link
Member Author

andydotxyz commented Aug 27, 2020

Let's take a look separately. A change in shadow calculation will change every widget image test with a shadow - if this PR inlcuded that I suspect that there would be a number of questions?
Aditionally I think that in future theme update we will introduce "alt background" which will be used to make the difference clearer.

@andydotxyz andydotxyz deleted the feature/card branch August 27, 2020 07:26
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