Allow to set position for the pagination component #332

am · 2018-11-29T07:17:38Z

Description

Add the option to position the pagination component in relation to the images

Motivation and Context

Position:
At the moment the pagination has a fixed position at the bottom, in some cases, it would be helpful to be able to change this. The main options would be position on top or bottom of the images.

Overlay:
In some cases vertical spacing is critical, and the pagination component does takes it's part. Although we can turn it off, making this component just a hint that overlays the images can solve the issue in a simple way.

How Has This Been Tested?

All commands below ran without error.

yarn install
yarn build
yarn dev
yarn dev:build
yarn docs:build
yarn precommit
yarn test-coverage
yarn updateDocAssets

Screenshots:

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
- I have updated the documentation accordingly.
I have included a vue-play example (if this is a new feature)

quinnlangille · 2018-11-29T16:05:14Z

Hey @am, this is great! It seems like this will resolve #298.

We also have a feature request for moving dots above the slides (#313). The implementation would be very similar to this. Would you be interested in making the modifications to include this functionality?

If not, totally ok, just thought it would be a cool update

am · 2018-11-29T18:26:17Z

Hi @quinnlangille , thanks for the heads-up.

Matter of fact I got to start working on this one after seeing the #313 issue. I was misled by the issue since I thought the OP meant to place the pagination overlaying the image, only then, after reading carefully the issue I realized that it was a different request. Since overlaying the pagination was what I needed I went for it first.

I think this could be tackled by two different PR's. I do see the relation at implementation level, but from a user/configuration level splitting is preferred:

paginationOverlay: Boolean (true/false) Overlays the pagination over the images
paginationPosition: String ('top', 'bottom', 'top-bottom') Defines the position of the pagination in relation to the images.

Anyway, I can start working on the position feature. What are your thoughts? Two PR's or do you prefer to have a single one?

quinnlangille · 2018-11-29T20:49:00Z

On the implementation level, I was thinking of having one prop that takes a value for all three

pagniationPosition which accepts top, bottom, or overlay, the latter enabling what is currently included in this feature.

What do you think?

am · 2018-11-29T21:24:54Z

@quinnlangille I like it! Trying to clarify, on #313 there are 3 options: top, bottom, and top-bottom, where the last does look to be a bit awkward as I never saw a case where there the pagination is duplicated. For the sake of simplicity, I would prefer to keep only one instance of the pagination component, but it's not what the OP askes.

In any case, I would give the following options to be able to cover the entire y axis:

top
bottom
top-overlay
bottom-overlay

am · 2018-11-29T22:54:04Z

@quinnlangille I've pushed another commit that does what I've described. It's open to discussion, please let me know your thoughts.

josephting · 2018-11-30T00:31:56Z

This is really slick. I like it. I agree with @am about having duplicated pagination.

The original issue #313 had was the pagination being pushed below the fold so I think this will address this specific issue by having option to set pagination to the top so it'll be above the fold.

However, I'm kind of reluctant to add duplicated pagination as well. Perhaps enabling navigation instead is what we can suggest the OP to do.

quinnlangille · 2018-11-30T04:39:13Z

Yeah I agree @am @josephting, we definitely don't want the duplicated pagination. Atleast, not as a part of this feature! I'll pull and test the changes in the morning, but gave a quick look and code looks good - nice work!

ashleysimpson · 2018-12-04T18:58:57Z

docs/source/api/index.md

@@ -221,6 +221,13 @@ The fill color of pagination dots. Any valid CSS color is accepted.
 * **Type**: `String`
 * **Default**: `#efefef`

+### paginationPosition
+
+The position of pagination dots. Possible values are 'top', 'top-overlay', 'bottom-overlay'.


A little nitpick but "bottom" should be a possible value

Weird, the reply feature doesn't reply to the message but adds a comment in the PR. Anyway, it's not a big deal. I know it's a default, I just personally like to add all the cases for completeness 😛

Actually, one thing I will mention is that you are missing the docs in the README.

@ashleysimpson thanks for the feedback! It was my first attempt to reply using email (and probably my last seeing the limitations it has).

So regarding your comments, to be honest, I have no opinion about that and I prefer to do what best matches the project. I'll be adding a commit ASAP.

Sounds good! And thanks for the contribution 😃

Np. I've added a commit that should cover your feedback. Please let me know if something is missing.

Thank you

am · 2018-12-04T19:05:49Z

It's the default, not setting any value will default to bottom.

…

On Tue, Dec 4, 2018, 8:01 PM Ashley Simpson ***@***.*** wrote: ***@***.**** commented on this pull request. ------------------------------ In docs/source/api/index.md <#332 (comment)>: > @@ -221,6 +221,13 @@ The fill color of pagination dots. Any valid CSS color is accepted. * **Type**: `String` * **Default**: `#efefef` +### paginationPosition + +The position of pagination dots. Possible values are 'top', 'top-overlay', 'bottom-overlay'. A little nitpick but "bottom" should be a possible value — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#332 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACEK4wTw1Kkvre-x1bqmySJYIwb0HvKks5u1sangaJpZM4Y5FAE> .

quinnlangille · 2018-12-05T21:26:58Z

Perfect, looks great! I'll merge into our working brand 0.17.0 and it will be released with the next minor 🚀

* feat(config): allow to overlay pagination component * feat(config): allow to set the pagination position * docs(config): add pagination position * docs(): improve paginationPosition description and add to README

FrankyBoy · 2021-06-02T12:12:23Z

So, what ever happened to this?

am changed the title ~~feat(config): allow to overlay pagination component~~ Allow to overlay pagination component Nov 29, 2018

feat(config): allow to overlay pagination component

6a8cd38

am force-pushed the feature/pagination-overlay branch from 8e6008b to 6a8cd38 Compare November 29, 2018 21:42

feat(config): allow to set the pagination position

7aec6df

docs(config): add pagination position

4fb8055

am changed the title ~~Allow to overlay pagination component~~ Allow to set position for the pagination component Nov 29, 2018

josephting approved these changes Nov 30, 2018

View reviewed changes

ashleysimpson reviewed Dec 4, 2018

View reviewed changes

ashleysimpson approved these changes Dec 4, 2018

View reviewed changes

docs(): improve paginationPosition description and add to README

89325fd

quinnlangille changed the base branch from master to v0.17.0 December 5, 2018 21:27

quinnlangille merged commit 78f2bbb into SSENSE:v0.17.0 Dec 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to set position for the pagination component #332

Allow to set position for the pagination component #332

am commented Nov 29, 2018 •

edited

Loading

quinnlangille commented Nov 29, 2018

am commented Nov 29, 2018

quinnlangille commented Nov 29, 2018

am commented Nov 29, 2018 •

edited

Loading

am commented Nov 29, 2018 •

edited

Loading

josephting commented Nov 30, 2018

quinnlangille commented Nov 30, 2018

ashleysimpson Dec 4, 2018

ashleysimpson Dec 4, 2018 •

edited

Loading

ashleysimpson Dec 4, 2018

am Dec 4, 2018

ashleysimpson Dec 4, 2018

am Dec 4, 2018

am commented Dec 4, 2018 via email

quinnlangille commented Dec 5, 2018

FrankyBoy commented Jun 2, 2021

Allow to set position for the pagination component #332

Allow to set position for the pagination component #332

Conversation

am commented Nov 29, 2018 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots:

Types of changes

Checklist:

quinnlangille commented Nov 29, 2018

am commented Nov 29, 2018

quinnlangille commented Nov 29, 2018

am commented Nov 29, 2018 • edited Loading

am commented Nov 29, 2018 • edited Loading

josephting commented Nov 30, 2018

quinnlangille commented Nov 30, 2018

ashleysimpson Dec 4, 2018

Choose a reason for hiding this comment

ashleysimpson Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

ashleysimpson Dec 4, 2018

Choose a reason for hiding this comment

am Dec 4, 2018

Choose a reason for hiding this comment

ashleysimpson Dec 4, 2018

Choose a reason for hiding this comment

am Dec 4, 2018

Choose a reason for hiding this comment

am commented Dec 4, 2018 via email

quinnlangille commented Dec 5, 2018

FrankyBoy commented Jun 2, 2021

am commented Nov 29, 2018 •

edited

Loading

am commented Nov 29, 2018 •

edited

Loading

am commented Nov 29, 2018 •

edited

Loading

ashleysimpson Dec 4, 2018 •

edited

Loading