-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
…xt with content and image header Any of these can be omitted as the tests verify
func (c *CardContainer) SetTitle(text string) { | ||
c.Title = text | ||
|
||
c.Refresh() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this 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
Closed in favour of #1263, will try to incorporate comments there, though questions here I will answer |
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. |
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? |
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: