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

[Carousel] Add an optional label to indicator buttons #4317

Merged
merged 6 commits into from
Apr 30, 2017
Merged

[Carousel] Add an optional label to indicator buttons #4317

merged 6 commits into from
Apr 30, 2017

Conversation

paul-blundell
Copy link
Contributor

@paul-blundell paul-blundell commented Apr 19, 2017

Please makes sure these boxes are checked before submitting your PR, thank you!

  • Make sure you follow Element's contributing guide
  • Make sure you are merging your commits to dev branch.
  • Add some descriptions and refer relative issues for you PR.

Adds an optional label to the indicator buttons on the carousel. Can be used only if required like so:

<n-carousel-item label="Slide 1">
  <h3>My Slide</h3>
</n-carousel-item>

Copy link
Contributor

@Leopoldthecoder Leopoldthecoder left a comment

Choose a reason for hiding this comment

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

image

As you can see from the above, I'm afraid this feature needs more than two lines of code. You can keep updating this PR if you're interested.

@paul-blundell
Copy link
Contributor Author

That was a fair point, I didn't think about the default theme.

I have added a new prop (defaults to false) for indicator-labels on the carousel itself. If true then it applies another class to the button and adds the label text from carousel-item.

Please let me know your thoughts.

@Leopoldthecoder
Copy link
Contributor

image

I tried the new commits and got the above. It doesn't seem to be desired.

To get what you proposed in #4153, I think more CSS needs to be added. For example, if indicator-labels is true, another class should be applied to <ul> to deal with its positioning and width. Besides, since the indicators can be both inside and outside the slides (governed by indicator-position), it makes sense that labelled indicators should look nice in both scenarios.

What do you think? Could you update this PR?

@paul-blundell
Copy link
Contributor Author

Updated to fix that issue. Added a class to the <ul> instead like you suggested.
There is still some overlap on smaller screens when you have a lot of carousel items, but maybe this should be down to the user to style as required or limit the number of items?

@Leopoldthecoder
Copy link
Contributor

I updated this. Removed indicatorLabels because we can check if items have labels inside main.vue.

@Leopoldthecoder Leopoldthecoder merged commit 54e2047 into ElemeFE:dev Apr 30, 2017
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.

None yet

3 participants