Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

new progressbar component of claim branch && more values available #2060

Merged
merged 5 commits into from
Jan 17, 2022

Conversation

maria-vslvn
Copy link
Contributor

Summary

Fixes #2008

New Progress Bar component was created, values can be set by clicking the numbers at the top of the bar or via dragging the bar or clicking at any bar point.
image

To Test

  1. Open the page Profile
    The bar is at the bottom of the form

@maria-vslvn maria-vslvn self-assigned this Jan 7, 2022
@alongoni alongoni added app:CowSwap CowSwap app Protofire Handled by Protofire development team labels Jan 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2022

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI


export const ProgressContainer = styled.div`
background-color: ${({ theme }) => transparentize(0.61, theme.text1)};
height: 16px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @maria-vslvn, I love it! Works fine!
Maybe we can add 24px height in order to have more space inside the bar and increase the font size of the variable %. (to font-size: 16px; and font-weight: 900;)

Another thing, we can decrease a bit the font-size of the values at the top (to 12px).

It'll look like this:
image

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Excellent work Maria, looks perfect!

While it does make sense for the component in isolation that's not how we envision the component to be used regarding the state handling.

Could you please remove the internal state and add a callback as a parameter like in the proposed API on the issue #2008 (comment) named onPercentageClick or onClick.

Then, instead of using the setProgressVal(value) you'd call onPercentageClick(value).

The parent component is the who'll hold the state and update the ProgressBar component by passing the updated value component.

You can have a test parent component, but it's not required for the current ProgressBar component.

Let me know if you have any questions

@maria-vslvn
Copy link
Contributor Author

@alfetopito please check the updates

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

You are very close!

@elena-zh
Copy link

Scrollbar LGTM itself, but is this OK that it becomes 100% filled when I select any option, including 0%?
And it is impossible to change a value then
image

@anxolin
Copy link
Contributor

anxolin commented Jan 13, 2022

It would be great to have this PR merged soonish, we are arriving the point where we need it

@ramirotw
Copy link
Contributor

It would be great to have this PR merged soonish, we are arriving the point where we need it

please take a look in the Profile page

Scrollbar LGTM itself, but is this OK that it becomes 100% filled when I select any option, including 0%?
And it is impossible to change a value then

the issue should be fixed

Copy link
Contributor

@alongoni alongoni left a comment

Choose a reason for hiding this comment

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

Great job!

@elena-zh
Copy link

Changes LGTM!

@ramirotw ramirotw added the Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds label Jan 17, 2022
@mergify mergify bot merged commit 00195a9 into claim Jan 17, 2022
@ramirotw ramirotw deleted the feature/2008-progressBar-component branch January 17, 2022 16:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
app:CowSwap CowSwap app Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds Protofire Handled by Protofire development team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants