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

Rewards/ViewReward: add status info #1640

Merged
merged 4 commits into from
Nov 22, 2019
Merged

Rewards/ViewReward: add status info #1640

merged 4 commits into from
Nov 22, 2019

Conversation

e18r
Copy link
Contributor

@e18r e18r commented Nov 14, 2019

TO DO before merging

Fixes #1629

Screenshot from 2019-11-14 09-04-48

@e18r e18r changed the title Rewards status info Rewards/ViewReward: add status info Nov 14, 2019
e18r added a commit that referenced this pull request Nov 15, 2019
**TO DO before merging**
- [ ] wait for PR #1640 to be merged
- [ ] redirect this PR to `dev`

Fixes #1631
@e18r e18r changed the base branch from rewards-icons-status to dev November 20, 2019 20:44
@e18r e18r requested a review from chadoh November 20, 2019 20:45
Copy link
Collaborator

@chadoh chadoh left a comment

Choose a reason for hiding this comment

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

Looks pretty good! Thanks for the cleanup. I see a couple quick things that I'd prefer we improved before merging, but nothing big. Feel free to push back on these suggestions!

apps/rewards/app/components/App/App.js Outdated Show resolved Hide resolved
apps/rewards/app/components/Panel/PanelManager.js Outdated Show resolved Hide resolved
const Header = styled(Text.Block).attrs({
smallcaps: true,
})`
color: ${({ theme }) => theme.contentSecondary};
Copy link
Collaborator

@chadoh chadoh Nov 21, 2019

Choose a reason for hiding this comment

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

Sort of a separate discussion, but I wonder if it makes more sense to use inline css= props for components like this, which would let us easily stop passing in theme and allow the component to define it itself:

import React from 'react'
import { GU, Text, theme } from '@aragon/ui'

export default function Header({ children }) {
  const theme = useTheme()
  return (
    <Text.Block
      smallcaps
      css={`color: ${theme.contentSecondary}; margin-top: ${2 * GU}px; margin-bottom: ${GU}px`}
    >
      {children}
    </Text.Block>
  )
}

A trade-off is that stylelint doesn't currently inspect these inline css= props correctly.

What do you think?

(Also, rewriting this made me think that we should probably use GU here.)

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 like your suggestion of making this a fully-fledged component. However, I had to use style instead of css.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I find it very strange that wrapping it in styled() works, but using inline css= doesn't. I thought they did the same thing!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤷‍♀️

apps/rewards/app/utils/helpers.js Outdated Show resolved Hide resolved
apps/rewards/app/components/Content/MyRewards.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@chadoh chadoh left a comment

Choose a reason for hiding this comment

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

💋

@e18r e18r merged commit 47bf53f into dev Nov 22, 2019
e18r added a commit that referenced this pull request Nov 25, 2019
e18r added a commit that referenced this pull request Nov 26, 2019
@e18r e18r deleted the rewards-status-info branch November 26, 2019 21:45
e18r added a commit that referenced this pull request Nov 27, 2019
e18r added a commit that referenced this pull request Nov 27, 2019
e18r added a commit that referenced this pull request Dec 10, 2019
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.

Rewards/ViewReward: add status information
2 participants