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

Table: Implement Classes Option #8349

Closed
wants to merge 83 commits into
base: 1.5-dev
from

Conversation

Projects
None yet
4 participants
@cgack
Contributor

cgack commented Dec 16, 2015

This includes the changes made in the table review branch https://github.com/jquery/jquery-mobile/tree/7360-table-review

gabrielschulhof added some commits May 6, 2014

Table: "create" parameter for _refresh() is only used in columntoggle
Thus, replace it with an instance variable turned on at the beginning of
_create() and turned off at the end of _create(). This removes the need for
_refresh(). refresh() can now handle the actual refresh, and rebuild() becomes
and alias for refresh(). We should deprecate rebuild() in favour of refresh().
Table: Make sure column is hidden for testing .prop( "checked", false )
Sometimes the column starts out as not visible, and sometimes it starts out as
visible. Before making sure that .prop( "checked", false ) has no effect, the
column must be rendered invisible irrespective of its starting state.
Table: Separate popup-related functionality
This introduces the extension and moves only the major portions.
@cgack

This comment has been minimized.

Show comment
Hide comment
@cgack

cgack Dec 16, 2015

Contributor

TODOS

  • Implement Classes
  • use hasClasses, lacksClasses in tests
  • review QUnit.skip-ed tests
Contributor

cgack commented Dec 16, 2015

TODOS

  • Implement Classes
  • use hasClasses, lacksClasses in tests
  • review QUnit.skip-ed tests

cgack added some commits Dec 16, 2015

Table: Implement Classes Option
remove extra test

Fixes gh-8341
Table: Implement Classes Option
hasClasses in tests

Fixes gh-8341
Table: Implement Classes Option
hasclasses and lacksclasses to table tests

Fixes gh-8341

@cgack cgack changed the title from [WIP] Table: Implement Classes Option to Table: Implement Classes Option Jan 21, 2016

@cgack cgack assigned cgack and arschmitz and unassigned cgack Jan 21, 2016

@cgack

This comment has been minimized.

Show comment
Hide comment
@cgack

cgack Jan 21, 2016

Contributor

@arschmitz I think this is read for a round of review. Thanks!

Contributor

cgack commented Jan 21, 2016

@arschmitz I think this is read for a round of review. Thanks!

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jan 21, 2016

Member

on it

Member

arschmitz commented Jan 21, 2016

on it

Show outdated Hide outdated js/widgets/table.columntoggle.js
this._on( this._menu, {
"change input": "_menuInputChange"
} );
_updateVariableColumn: function( header, cells, priority/*, state */ ) {

This comment has been minimized.

@arschmitz

arschmitz Jan 28, 2016

Member

remove unused stae var comment

@arschmitz

arschmitz Jan 28, 2016

Member

remove unused stae var comment

Show outdated Hide outdated js/widgets/table.columntoggle.js
this._super();
if ( this.options.mode === "columntoggle" ) {
if ( !this.options.enhanced ) {
this._removeClass( "ui-table-columntoggle" );

This comment has been minimized.

@arschmitz

arschmitz Jan 28, 2016

Member

no need to remove classes in destroy

@arschmitz

arschmitz Jan 28, 2016

Member

no need to remove classes in destroy

Show outdated Hide outdated js/widgets/table.columntoggle.js
header = $( element );
header
.add( header.jqmData( "cells" ) )
.removeClass( this.options.classes.priorityPrefix + priority );

This comment has been minimized.

@arschmitz

arschmitz Jan 28, 2016

Member

same here

@arschmitz

arschmitz Jan 28, 2016

Member

same here

Show outdated Hide outdated js/widgets/table.columntoggle.popup.js
columnUi: true,
classes: {
"ui-table-columntoggle-popup": "",
"ui-table-columntoggle-btn": "ui-button ui-corner-all ui-shadow ui-mini"

This comment has been minimized.

@arschmitz

arschmitz Jan 28, 2016

Member

ui-button is not a theme class

@arschmitz

arschmitz Jan 28, 2016

Member

ui-button is not a theme class

Show outdated Hide outdated js/widgets/table.js
var table = this.element;
if ( !this.options.enhanced ) {
this._removeClass( "ui-table" );

This comment has been minimized.

@arschmitz

arschmitz Jan 28, 2016

Member

no need to remove classes in destory

@arschmitz

arschmitz Jan 28, 2016

Member

no need to remove classes in destory

Show outdated Hide outdated js/widgets/table.reflow.js
.removeAttr( colstartAttr )
.end()
.end()
.removeClass( "ui-table-reflow" )

This comment has been minimized.

@arschmitz

arschmitz Jan 28, 2016

Member

use of old removeclass and its in destory

@arschmitz

arschmitz Jan 28, 2016

Member

use of old removeclass and its in destory

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jan 28, 2016

Member

A couple things i noticed throughout were use of jqmdata, jqmremovedata, and $.mobile.getAttribute all of which are deprecated. I also noticed there are not any backcompat tests over all looking good

Member

arschmitz commented Jan 28, 2016

A couple things i noticed throughout were use of jqmdata, jqmremovedata, and $.mobile.getAttribute all of which are deprecated. I also noticed there are not any backcompat tests over all looking good

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jan 28, 2016

Member

Oh and of course no hasClasses, but you have that as a todo already

Member

arschmitz commented Jan 28, 2016

Oh and of course no hasClasses, but you have that as a todo already

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jan 28, 2016

Member

Related to tests it looks like this fixes the failures we are seeing in 1.5-dev on table but the Basic table: _refreshHeaderCell() test is failing across the board

Member

arschmitz commented Jan 28, 2016

Related to tests it looks like this fixes the failures we are seeing in 1.5-dev on table but the Basic table: _refreshHeaderCell() test is failing across the board

cgack added some commits Feb 4, 2016

Table: Implment Classes Option
post review changes

Fixes gh-8341
@cgack

This comment has been minimized.

Show comment
Hide comment
@cgack

cgack Feb 18, 2016

Contributor

gh-6531 needs to have the table-stripe removed

Contributor

cgack commented Feb 18, 2016

gh-6531 needs to have the table-stripe removed

Table: Implement Classes Option
test adjustments

Fixes gh-8341

@arschmitz arschmitz referenced this pull request Mar 26, 2016

Closed

7360 table review #7372

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Mar 28, 2016

Member

rebased fixed and landed on 1.5-dev

Member

arschmitz commented Mar 28, 2016

rebased fixed and landed on 1.5-dev

@arschmitz arschmitz closed this Mar 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment