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

fix(VTabs): RTL support for VTabs #6812

Closed
wants to merge 1 commit into from

Conversation

ram33
Copy link

@ram33 ram33 commented Mar 23, 2019

Fixed few issues with VTabs in RTL mode.

Description

There are couple of issues with VTabs in RTL mode with pagination.
Initial tabs positioning not right. Tabs moved towards left onto pagination leaving empty space on right. - fixed
Prev/Next button clicks moving tabs in opposite direction - Fixed
Prev/Next buttons not hiding based on Tab scroll position - Fixed
Position active tab in visible portion of tabs on load - Not fixed.

Motivation and Context

We are using VTabs as menu bar in our application and without this Tabs are not functional/usable in RTL mode.

There are couple of issues logged. One of them is open and other are closed as no support at that moment or marked as fixed (but not actually fixed).

#6696
#4788
#5329
#6575

How Has This Been Tested?

I have used Playground.vue to test this.

None

Markup:

// dev/index.js
Vue.use(Vuetify, { rtl: true })

// Paste your FULL Playground.vue here
<template>
  <v-app>
    <v-tabs
      v-model="activeTabIndex"
      dark
      color="cyan"
      show-arrows
    >
      <v-tabs-slider color="yellow" />
      <v-tab
        v-for="i in 15"
        :key="i"
        :href="'#tab-' + i"
      >
        Item {{ i }}
      </v-tab>
      <v-tabs-items>
        <v-tab-item
          v-for="i in 15"
          :id="'tab-' + i"
          :key="i"
        >
          <v-card flat>
            <v-card-text>
              'Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor
              incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam,
              quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.'
            </v-card-text>
          </v-card>
        </v-tab-item>
      </v-tabs-items>
    </v-tabs>
  </v-app>
</template>

<script>
  export default {
    data () {
      return {
        activeTabIndex: 'tab-12'
      }
    }
  }
</script>

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)
  • Improvement/refactoring (non-breaking change that doesn't add any feature but make things better)

Checklist:

  • The PR title is no longer than 64 characters.
  • The PR is submitted to the correct branch (master for bug fixes and documentation updates, dev for new features and breaking changes).
  • My code follows the code style of this project.
  • I've added relevant changes to the documentation (applies to new features and breaking changes in core library)

Fixed few issues with VTabs in RTL mode.
@codecov
Copy link

codecov bot commented Mar 23, 2019

Codecov Report

Merging #6812 into master will decrease coverage by 0.09%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #6812     +/-   ##
=========================================
- Coverage   85.31%   85.22%   -0.1%     
=========================================
  Files         298      298             
  Lines        7210     7220     +10     
  Branches     1803     1810      +7     
=========================================
+ Hits         6151     6153      +2     
- Misses        958      962      +4     
- Partials      101      105      +4
Impacted Files Coverage Δ
packages/vuetify/src/components/VTabs/VTabs.js 100% <100%> (ø) ⬆️
.../vuetify/src/components/VTabs/mixins/tabs-touch.js 47.82% <11.11%> (-27.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 175b9ac...3a702a6. Read the comment docs.

@vercel
Copy link

vercel bot commented Mar 23, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Copy link
Member

@MajesticPotatoe MajesticPotatoe left a comment

Choose a reason for hiding this comment

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

  • This should be first rebased to next.
  • Secondly you need to provide you full playground in the description so we are able to test.
  • Third vtabs have already been converted to MD2 with real support so before (hence why the issues that were closed but don't seem fixed are there, as they have been fixed in 2.0)
    I would suggest looking at what 2.0 has before moving forward with this. For the time being I'm marking this on hold and for review.

@MajesticPotatoe MajesticPotatoe added S: on hold The issue is on hold until further notice pending team review The issue is pending a full team review S: needs reproduction The issue does not contain a valid reproduction labels Mar 23, 2019
@jacekkarczmarczyk
Copy link
Member

This should be first rebased to dev.

*next

@ram33
Copy link
Author

ram33 commented Mar 24, 2019

@MajesticPotatoe Thank you for your review...

This should be first rebased to dev.

  • This code change was originally made on top of dev branch only. But in the documentation it was mentioned to raise pull-request for master, if it's a fix and to dev, if its a feature or breaking change.

Secondly you need to provide you full playground in the description so we are able to test.

  • I believe, I have provided whole playgroud.vue. I have missed to set rtl mode. I have updated description with it now. Please let me know, if I missed anything else.

Third vtabs have already been converted to MD2 with real support so before (hence why the issues that were closed but don't seem fixed are there, as they have been fixed in 2.0)
I would suggest looking at what 2.0 has before moving forward with this. For the time being I'm marking this on hold and for review.

  • I have tested this in 2.0 before fixing it and same issues still persist there. Here is codepen

@ram33
Copy link
Author

ram33 commented Mar 28, 2019

@MajesticPotatoe Any further action required from me to get this going?

@MajesticPotatoe
Copy link
Member

@ram33 reviewing with team. In the meantime please rebase this to next.

@MajesticPotatoe MajesticPotatoe removed the S: needs reproduction The issue does not contain a valid reproduction label Mar 29, 2019
@johnleider
Copy link
Member

This Pull Request is being closed due to inactivity.

Thank you for your contribution and interest in improving Vuetify!

@johnleider johnleider closed this Apr 6, 2019
@ram33
Copy link
Author

ram33 commented Apr 8, 2019

@johnleider So all the pull-requests should be rebased to next branch only... can't we get this fix on 1.5.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending team review The issue is pending a full team review S: on hold The issue is on hold until further notice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants