fix(VList/VListGroup): add missing rtl margins and paddings (v2.0) #7861

melbahja · 2019-07-18T00:14:47Z

fixes #8125

Hi,

I was testing vuetify v2 and I find paddings and margins not exists for rtl, So i decided to open this PR.

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 features but makes 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)

I'm happy to follow up and send more PRs if I find something.
Thank you :)

jacekkarczmarczyk · 2019-07-18T05:21:46Z

Please add the playground code for testing

melbahja · 2019-07-19T16:44:19Z

Hi, @jacekkarczmarczyk
test it on vuetify RTL version

<template>
   <div>
      <v-card
         max-width="500"
         class="mx-auto"
       >
         <v-toolbar
           color="deep-purple accent-4"
           dark
         >
           <v-app-bar-nav-icon></v-app-bar-nav-icon>

           <v-toolbar-title>New Chat</v-toolbar-title>

           <v-spacer></v-spacer>

           <v-btn icon>
             <v-icon>search</v-icon>
           </v-btn>
         </v-toolbar>

         <v-list subheader>
           <v-subheader>Recent chat</v-subheader>

           <v-list-item
             v-for="item in items"
             :key="item.title"
             @click=""
           >
             <v-list-item-avatar>
               <v-img :src="item.avatar"></v-img>
             </v-list-item-avatar>

             <v-list-item-content>
               <v-list-item-title v-text="item.title"></v-list-item-title>
             </v-list-item-content>

             <v-list-item-icon>
               <v-icon :color="item.active ? 'deep-purple accent-4' : 'grey'">chat_bubble</v-icon>
             </v-list-item-icon>
           </v-list-item>
         </v-list>

         <v-divider></v-divider>

         <v-list subheader>
           <v-subheader>Previous chats</v-subheader>

           <v-list-item
             v-for="item in items2"
             :key="item.title"
             @click=""
           >
             <v-list-item-avatar>
               <v-img :src="item.avatar"></v-img>
             </v-list-item-avatar>

             <v-list-item-content>
               <v-list-item-title v-text="item.title"></v-list-item-title>
             </v-list-item-content>

             <v-list-item-avatar>
               <v-img :src="item.avatar"></v-img>
             </v-list-item-avatar>

           </v-list-item>
         </v-list>
       </v-card>

       <br><br>
       <v-card
         class="mx-auto"
         width="300"
       >
         <v-list>
           <v-list-item>
             <v-list-item-icon>
               <v-icon>home</v-icon>
             </v-list-item-icon>

             <v-list-item-title>Home</v-list-item-title>
           </v-list-item>

           <v-list-group
             prepend-icon="account_circle"
             value="true"
           >
             <template v-slot:activator>
               <v-list-item>
                 <v-list-item-title>Users</v-list-item-title>
               </v-list-item>
             </template>

             <v-list-group
               no-action
               sub-group
               value="true"
             >
               <template v-slot:activator>
                 <v-list-item>
                   <v-list-item-content>
                     <v-list-item-title>Admin</v-list-item-title>
                   </v-list-item-content>
                 </v-list-item>
               </template>

               <v-list-item
                 v-for="(admin, i) in admins"
                 :key="i"
                 link
               >
                 <v-list-item-title v-text="admin[0]"></v-list-item-title>
                 <v-list-item-icon>
                   <v-icon v-text="admin[1]"></v-icon>
                 </v-list-item-icon>
               </v-list-item>
             </v-list-group>

             <v-list-group
               sub-group
               no-action
             >
               <template v-slot:activator>
                 <v-list-item>
                   <v-list-item-content>
                     <v-list-item-title>Actions</v-list-item-title>
                   </v-list-item-content>
                 </v-list-item>
               </template>
               <v-list-item
                 v-for="(crud, i) in cruds"
                 :key="i"
                 @click=""
               >
                 <v-list-item-title v-text="crud[0]"></v-list-item-title>
                 <v-list-item-action>
                   <v-icon v-text="crud[1]"></v-icon>
                 </v-list-item-action>
               </v-list-item>
             </v-list-group>
           </v-list-group>
         </v-list>
       </v-card>

       <br>
       <v-card
         class="mx-auto"
         color="#26c6da"
         dark
         max-width="400"
       >
         <v-card-title>
           <v-icon
             large
             right
           >
             mdi-twitter
           </v-icon>
           <span class="title font-weight-light">Twitter</span>
         </v-card-title>

         <v-card-text class="headline font-weight-bold">
           "Turns out semicolon-less style is easier and safer in TS because most gotcha edge cases are type invalid as well."
         </v-card-text>

         <v-card-actions>
           <v-list-item class="grow">
             <v-list-item-avatar color="grey darken-3">
               <v-img
                 class="elevation-6"
                 src="https://avataaars.io/?avatarStyle=Transparent&topType=ShortHairShortCurly&accessoriesType=Prescription02&hairColor=Black&facialHairType=Blank&clotheType=Hoodie&clotheColor=White&eyeType=Default&eyebrowType=DefaultNatural&mouthType=Default&skinColor=Light"
               ></v-img>
             </v-list-item-avatar>

             <v-list-item-content>
               <v-list-item-title>Evan You</v-list-item-title>
             </v-list-item-content>

             <v-layout
               align-center
               justify-end
             >
               <v-icon class="mr-1">mdi-heart</v-icon>
               <span class="subheading mr-2">256</span>
               <span class="mr-1">·</span>
               <v-icon class="mr-1">mdi-share-variant</v-icon>
               <span class="subheading">45</span>
             </v-layout>
           </v-list-item>
         </v-card-actions>
       </v-card>
       
   </div>
</template>


<script>
export default 
{
  data: () => (
  {
    items: [
        { active: true, title: 'Jason Oner', avatar: 'https://cdn.vuetifyjs.com/images/lists/1.jpg' },
        { active: true, title: 'Ranee Carlson', avatar: 'https://cdn.vuetifyjs.com/images/lists/2.jpg' },
        { title: 'Cindy Baker', avatar: 'https://cdn.vuetifyjs.com/images/lists/3.jpg' },
        { title: 'Ali Connors', avatar: 'https://cdn.vuetifyjs.com/images/lists/4.jpg' },
      ],
      items2: [
        { title: 'Travis Howard', avatar: 'https://cdn.vuetifyjs.com/images/lists/5.jpg' },
      ],

      admins: [
        ['Management', 'people_outline'],
        ['Settings', 'settings'],
      ],
      cruds: [
        ['Create', 'add'],
        ['Read', 'insert_drive_file'],
        ['Update', 'update'],
        ['Delete', 'delete'],
      ],
    
  }),
};
</script>

melbahja · 2019-07-22T00:07:31Z

I was playing with RTL VTabs, I found issue with slide touch (onTouchEnd event in VSlideGroup.ts)
You can understand what I'm talking about here:
https://codepen.io/dev0x0/full/dxoxqq
I solved the issue with this simple solution in VSlideGroup.ts line 261, I replaced scrollOffset if else with:

            if (this.$vuetify.rtl) {

                /* istanbul ignore else */
                if (this.scrollOffset > 0 || !this.isOverflowing) {
                    this.scrollOffset = 0;
                } else if (this.scrollOffset <= -maxScrollOffset) {
                    this.scrollOffset = -maxScrollOffset;
                }
            
            } else {

                /* istanbul ignore else */
                if (this.scrollOffset < 0 || !this.isOverflowing) {
                    this.scrollOffset = 0;
                } else if (this.scrollOffset >= maxScrollOffset) {
                    this.scrollOffset = maxScrollOffset;
                }
            }

@jacekkarczmarczyk @johnleider should i open a new PR or add a commit here
thank you :)

jacekkarczmarczyk · 2019-07-22T04:51:30Z

New PR. Ty

codecov · 2019-07-28T22:01:28Z

Codecov Report

Merging #7861 into master will decrease coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7861      +/-   ##
==========================================
- Coverage    85.8%   85.65%   -0.15%     
==========================================
  Files         334      333       -1     
  Lines        9095     9091       -4     
  Branches     2418     2414       -4     
==========================================
- Hits         7804     7787      -17     
- Misses       1203     1211       +8     
- Partials       88       93       +5

Impacted Files	Coverage Δ
packages/vuetify/src/util/mergeData.ts	`58.82% <0%> (-17.65%)`	⬇️
packages/vuetify/src/components/VTabs/VTabs.ts	`83.75% <0%> (-6.25%)`	⬇️
...es/vuetify/src/components/VItemGroup/VItemGroup.ts	`95.34% <0%> (-4.66%)`	⬇️
...ackages/vuetify/src/components/VGrid/VContainer.ts	`55% <0%> (-2.15%)`	⬇️
...rc/components/VDataTable/VDataTableHeaderMobile.ts	`93.54% <0%> (-0.21%)`	⬇️
.../vuetify/src/components/VPagination/VPagination.ts	`100% <0%> (ø)`	⬆️
packages/vuetify/src/components/VGrid/VRow.ts	`0% <0%> (ø)`	⬆️
...es/vuetify/src/components/VSparkline/VSparkline.ts	`73.33% <0%> (ø)`	⬆️
...es/vuetify/src/components/VSystemBar/VSystemBar.ts	`94.11% <0%> (ø)`	⬆️
...es/vuetify/src/components/VDataTable/VDataTable.ts	`83.75% <0%> (ø)`	⬆️
... and 9 more

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 1bfb4ad...9f6acb8. Read the comment docs.

packages/vuetify/src/components/VList/VListGroup.sass

TheInvoker · 2019-08-08T18:49:22Z

was this change released to public already?

melbahja · 2019-08-08T20:11:39Z

@TheInvoker yes check v2.0.4
https://github.com/vuetifyjs/vuetify/releases/tag/v2.0.4

melbahja added 2 commits July 18, 2019 00:57

adding rtl padding and margin

c87820a

add rtl margin

cda869a

vercel bot deployed to staging July 18, 2019 00:14 View deployment

melbahja changed the title ~~Add RTL margins and paddings for VList~~ Add RTL margins and paddings for VList (v2.0) Jul 18, 2019

johnleider added the S: on hold The issue is on hold until further notice label Jul 18, 2019

Update VListItem.sass

db7f2e0

vercel bot deployed to staging July 19, 2019 16:45 View deployment

Merge branch 'next' into next

e2976cd

vercel bot deployed to staging July 22, 2019 23:40 View deployment

jacekkarczmarczyk added rtl and removed S: on hold The issue is on hold until further notice labels Jul 28, 2019

This comment has been minimized.

Sign in to view

KaelWD changed the base branch from next to master July 28, 2019 09:00

jacekkarczmarczyk added this to the v2.0.x milestone Jul 28, 2019

jacekkarczmarczyk added T: bug Functionality that does not work as intended/expected S: has merge conflicts The pending Pull Request has merge conflicts labels Jul 28, 2019

jacekkarczmarczyk mentioned this pull request Jul 28, 2019

[Bug Report] RTL: v-list-item-action has margin-right instead of margin-left #8125

Closed

Merge branch 'master' into next

9f6acb8

vercel bot deployed to staging July 28, 2019 22:01 View deployment

jacekkarczmarczyk removed the S: has merge conflicts The pending Pull Request has merge conflicts label Jul 28, 2019

jacekkarczmarczyk reviewed Jul 29, 2019

View reviewed changes

packages/vuetify/src/components/VList/VListGroup.sass Show resolved Hide resolved

Add ltr function

0910627

vercel bot temporarily deployed to staging July 29, 2019 11:48 Inactive

Merge branch 'master' into next

9c34b4b

vercel bot deployed to staging July 29, 2019 11:53 View deployment

jacekkarczmarczyk approved these changes Jul 29, 2019

View reviewed changes

jacekkarczmarczyk mentioned this pull request Aug 1, 2019

[Bug Report] No margin between the prepend-icon and v-text-field / v-select when RTL is enabled #8241

Closed

johnleider changed the title ~~Add RTL margins and paddings for VList (v2.0)~~ fix(VList/VListGroup): add missing rtl margins and paddings (v2.0) Aug 1, 2019

johnleider added C: VList VList C: VListGroup VListGroup labels Aug 1, 2019

johnleider merged commit e1ba658 into vuetifyjs:master Aug 1, 2019

KaelWD mentioned this pull request Aug 3, 2019

[Bug Report] Spacing is not correct with RTL for list items #8278

Closed

lock bot locked as resolved and limited conversation to collaborators Sep 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(VList/VListGroup): add missing rtl margins and paddings (v2.0) #7861

fix(VList/VListGroup): add missing rtl margins and paddings (v2.0) #7861

melbahja commented Jul 18, 2019 •

edited by jacekkarczmarczyk

Loading

jacekkarczmarczyk commented Jul 18, 2019

melbahja commented Jul 19, 2019

melbahja commented Jul 22, 2019

jacekkarczmarczyk commented Jul 22, 2019

This comment has been minimized.

codecov bot commented Jul 28, 2019

TheInvoker commented Aug 8, 2019

melbahja commented Aug 8, 2019

fix(VList/VListGroup): add missing rtl margins and paddings (v2.0) #7861

fix(VList/VListGroup): add missing rtl margins and paddings (v2.0) #7861

Conversation

melbahja commented Jul 18, 2019 • edited by jacekkarczmarczyk Loading

Types of changes

Checklist:

jacekkarczmarczyk commented Jul 18, 2019

melbahja commented Jul 19, 2019

melbahja commented Jul 22, 2019

jacekkarczmarczyk commented Jul 22, 2019

This comment has been minimized.

codecov bot commented Jul 28, 2019

Codecov Report

TheInvoker commented Aug 8, 2019

melbahja commented Aug 8, 2019

melbahja commented Jul 18, 2019 •

edited by jacekkarczmarczyk

Loading