Skip to content

Conversation

@ksraj123
Copy link
Contributor

@ksraj123 ksraj123 commented Jul 7, 2020

fixes #500 and #498

A new component is introduced in the common directory which is a wrapper around react-bootstrap primary Button and can accept all the props as bootstrap Button but remains its concerned style properties remain unaffected to styles introduced via style prop or style sheets (except when !important). CSS code made redundant due to this is removed. In few places where introducing this component might break the app, simply styles has been changed.

Is the styling as per what is wanted? only done the dashboard page, will do the rest if its alright.
Please share feedback and suggestions.

Screenshot from 2020-07-07 20-19-01
Screenshot from 2020-07-07 20-19-06

@netlify
Copy link

netlify bot commented Jul 7, 2020

Deploy preview for donut-frontend ready!

Built with commit e0542fc

https://deploy-preview-506--donut-frontend.netlify.app

@ksraj123 ksraj123 changed the title [WIP] Fixes #500 - Uniform Button styles [WIP] Fixes #500 #498 Uniform Button styles Jul 7, 2020
Copy link
Member

@Rupeshiya Rupeshiya left a comment

Choose a reason for hiding this comment

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

@ksraj123 Please check comments.
Also the button UI is not looking aligned with the fields.

<p>By {project?.projectOwner || "CODEUINO"}</p>
<div className="view-project">
<Button className="view-project-btn">View Project</Button>
<Btn className="view-project-btn">View Project</Btn>
Copy link
Member

Choose a reason for hiding this comment

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

Hey @ksraj123,
We are using react-bootstrap throughout the platform. So we should use the same for the button itself and then customize it as per our design for the whole platform not only dashboard.

Copy link
Contributor Author

@ksraj123 ksraj123 Jul 8, 2020

Choose a reason for hiding this comment

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

The Btn component is based on react-bootstrap Button component, Please have a look at src/common/Button.js its just a wrapper around that which makes sure the styling attributes which matter to us (i.e. fontFamily, boxShadow, borderRadius, backgroundColor and color) for maintaining uniformity in style cannot be changed through styles passed through props or external stylesheets or even bootstrap related props (variant, bsPrefix etc) . The other way to do it would be to manually change css across the project but we would have to rely on new developers to be diligent to ensure that new buttons added are styled similarly. I felt having a common component which could be used to get a button as per the style requirements of the project would be an elegant solution to this. Thanks. Would love to know what you think.

And as I mentioned in the body of my PR, I have done this only for the dashboard now to get feedback on if this is the design that's wanted. If so would do it in the entire project. Please go through the PR body. Thanks.

Copy link
Member

@Rupeshiya Rupeshiya Jul 8, 2020

Choose a reason for hiding this comment

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

yeah, I appreciate the way you have written the code as it's flexible and can be used anywhere.
But as the classes are already defined there, so you can simply modify there, because component style will overwrite that.

Copy link
Member

Choose a reason for hiding this comment

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

Also this PR is changing all the button to rectangular shape, but that's not expected, like look at the tags of notification it should not be rectangular. Also that tabs in dashboard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this PR is changing all the button to rectangular shape, but that's not expected, like look at the tags of notification it should not be rectangular. Also that tabs in dashboard

@Rupeshiya Thank you for the review and suggestions,

Restored changes made to tags and section buttons, also aligned things a little, please see if it looks good now.

But as the classes are already defined there, so you can simply modify there, because component style will overwrite that.

Also, about the Btn component should I scrap that and just modify styles everywhere? Thanks.

Screenshot from 2020-07-09 04-30-1911

.tag-btn {
background: #1a73e8;
border-radius: 15px;
font-family: Inter;
Copy link
Member

Choose a reason for hiding this comment

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

also, we are having font-family Inter for almost all the texts we are using on donut.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its been removed because that CSS code is redundant now. Our Btn comoponet will always have the following styles

        fontFamily: 'Inter',
        boxShadow: 'none',
        borderRadius: '0.25rem',
        fontStyle: 'normal'
        backgroundColor = '#1A73E8';
        color = '#ffffff';

the backgroundColor and color would interchange if outline btnType is selected.

Please go though declaration of the component in src/common/Button.js.
Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

yup

@ksraj123 ksraj123 requested a review from Rupeshiya July 8, 2020 15:02
@jaskiratsingh2000
Copy link
Member

@vaibhavdaren @Rupeshiya ?

@jaskiratsingh2000
Copy link
Member

Since this is done using redux , node.js

display: flex;
align-items: center;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

.

@ksraj123
Copy link
Contributor Author

Being fixed in #515

@ksraj123 ksraj123 closed this Jul 12, 2020
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.

4 participants