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

Add React component for pagination bullets #1261

Closed
wants to merge 3 commits into from

Conversation

ludamillion
Copy link
Contributor

Description

Does what it says on the tin. Adds a React component for pagination bullets.

There was no hover state specified in the Figma designs so I reused the hover states from the buttons which share the same color scheme.

I'm new to React and mostly implemented this based on analyzing the other components and reading the docs.

Corresponding Issue

#848

Screenshots

screenshot_2018-11-12 storybook

screenshot_2018-11-12 storybook 1

Test Coverage

Copy link
Member

@julianguyen julianguyen left a comment

Choose a reason for hiding this comment

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

Great progress so far! :) Thanks for working on this and yay for writing tests. So we actually want to be using the bullet element in HTML for this. See comments :)

Let me know if you have any questions or need any help!


&.normal {
color: $white;
background-color: rgba(255, 255, 255, 0.1);
Copy link
Member

Choose a reason for hiding this comment

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

You can use $white-10 instead :)

@@ -13,6 +13,7 @@ import { Header } from '../components/Header';
import { HeaderProfile } from '../components/Header/HeaderProfile';
import { Resource } from '../components/Resource';
import { Tag } from '../components/Tag';
import { Bullet } from '../components/Bullet';
Copy link
Member

Choose a reason for hiding this comment

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

You can remove this since the component isn't being used in any Rails views yet.

} = props;
const labelClassNames = `bullet ${css.bullet} ${active ? css.active : ''}
${normal ? css.normal : ''}`;
return <div className={labelClassNames}><span>{label}</span></div>;
Copy link
Member

Choose a reason for hiding this comment

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

So we actually want to build this component using <li>, and style it accordingly depending on whether the parent is <ul> or <ol>.

@julianguyen
Copy link
Member

I thought about this more, and we actually don't need a React component for this. We just need to style our default CSS for li, ul, and ol.

@ludamillion
Copy link
Contributor Author

I'll take another swing at simply styling the existing pagination elements. It looks like the existing pagination bullets/links are not accounted for in the design docs so I'll take a design pass at that as well. The pagination in the design docs is against the dark background which does not work in the current use case.

@ludamillion
Copy link
Contributor Author

Here's a super quick treatment I threw together based on the other pagination.
quick-treatment

@julianguyen
Copy link
Member

Nice work with the pagination items! I think you can actually rename that existing React component you created as PaginationItem.

Then for the bullets, just override the default ul, ol, and li styles :)

Thanks for working on this :D

@hmasila
Copy link
Contributor

hmasila commented Nov 20, 2018

Woow, this is amazing 🎉

@paschalidi
Copy link
Member

@ludamillion thats an amazing work. I do not know if this is still in progress but if you have no time I could go on and start developing it again. Please let me know how you feel about it. @julianguyen you may know the status of this?

@julianguyen
Copy link
Member

@paschalidi I don't know the status, unfortunately, but yeah good idea to check in and ask Luke!

Happy to help you look for another issue to start off with :P Let me know on Slack!

@ludamillion
Copy link
Contributor Author

@paschalidi Feel free to take over on this one. I started working on it just as I was starting a new job and haven't really been able to make time for working on ifme yet.

@paschalidi
Copy link
Member

@ludamillion @julianguyen will work on that only after I finish #1390 & #1391

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