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

Cem 2516 #380

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Cem 2516 #380

wants to merge 15 commits into from

Conversation

percypie
Copy link

No description provided.

…ver to branch and created folder for LoyaltyPoints in src, noted what I have to do this week.
…r. made changes ralph suggested during friday meeting that fixed colour and coding practice issues.
Copy link
Member

@ralph-dev ralph-dev left a comment

Choose a reason for hiding this comment

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

Use the Theme

@ralph-dev
Copy link
Member

https://github.com/cheapreats/react-ui-library/blob/master/src/Themes/MainTheme.ts

Theme lives here, feel free to extend it with the color white if its not there

…t guide, and corrected the errors pointed out by ralph on github.
Copy link
Member

@ralph-dev ralph-dev left a comment

Choose a reason for hiding this comment

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

Why is a random package added?

package.json Outdated Show resolved Hide resolved
@ralph-dev
Copy link
Member

Try removing it, then running npm install and npm start and see if application still work

Copy link
Member

@ralph-dev ralph-dev left a comment

Choose a reason for hiding this comment

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

Please include image in PR


export interface LoyaltyPointsProps
extends React.HTMLAttributes<HTMLDivElement>{
LoyaltyAmount: number;
Copy link
Member

Choose a reason for hiding this comment

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

Why are props uppercase? With no docs

Copy link
Author

Choose a reason for hiding this comment

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

Im having an issue with the text colour being forced to be the colour of paragraph rather than my styled div since changing to it, cant figure out how to fix that.

@percypie
Copy link
Author

percypie commented Nov 7, 2021

Screenshot 2021-11-07 104825

@ralph-dev
Copy link
Member

Have you tried overriding the styles? If so, maybe you need to specify a higher specificity

@percypie
Copy link
Author

percypie commented Nov 7, 2021

Yeah I tried this but it didnt work, is there another way to specify a higher specificity. also I've talked to Hunter about how he did it using styled.header, but from my understanding that doesnt work does it? Im pretty sure its Heading and not header, and you have to override it with brackets not a '.' right?
Screenshot 2021-11-07 180557

@ralph-dev
Copy link
Member

@percypie
In this case let's not re-use the paragraph component then, I'll explain why on our team call.

@percypie
Copy link
Author

percypie commented Nov 9, 2021

alright thank you, sorry I couldnt figure it out.

@percypie
Copy link
Author

percypie commented Nov 9, 2021

image

Copy link
Member

@ralph-dev ralph-dev left a comment

Choose a reason for hiding this comment

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

Lgtm

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants