Replace files app ids to classes

[PVince81]

Goal was to reduce id duplication due to the fact that file lists are duplicated in DOM for the different sections of the files app.

Based on apps/files/templates/list.php, then searched for all files in the workspace that reference #$var or 'var' or "var":

• #fileList

• #filestable

• #emptycontent

• #controls

• #notification <= likely important as it appears in multiple apps/files*/template/list.php files

• check if those appear multiple times

  • #dir
  • #permissions
  • #select-all-files ?

• probably unnecessary to replace:

  • #dirToken
  • #free_space
  • #uploadprogresswrapper

Further todos:

• fix failing JS tests NODE_PATH=build/node_modules build/node_modules/karma/bin/karma start tests/karma.config.js
• PR for text app to replace test selectors: Replace id to class for files app text#2745
• PR for viewer app to replace test selectors: Replace id to class for files app viewer#1298
• PR for activity app to replace test selectors: Rename fileList selector activity#857
• find other apps that extend the files app?

Tests:

• file listing works all left sidebar sections
• emptycontent in left sidebar sections
• files drop still works
• search results in files app still works
• test columns and sorting in various file lists (due to name change)
• recheck if Lighthouse in snapshot mode still reports duplicates after having clicked on various sections
• public link page + upload
• test activity
• test viewer
• test text app

[vinicius73]

It was considered to use data-* attributes instead of class?

[PVince81]

I don't think "data-*" attributes are adequate for addressing/querying elements, or at least less than classes

[vinicius73]

I don't think "data-*" attributes are adequate for addressing/querying elements, or at least less than classes

My fear is that are very simple and generic selectors. Maybe we can use prefix to prevent collisions.

• .nc-el--filelist
• .nc-server--filelist
• .nc-core--filelist
• .nc--filelist

[PVince81]

originally I wanted to keep the change minimal, considering that in the long term the file lists will be replaced with Vue components.

now, since you mention collisions, and since I already identified the locations to touch in the diff, it wouldn't hurt to use the opportunity to add a namespace. Thanks for bringing it up

[PVince81]

I've now renamed "filestable" to "files-filestable" and "fileList" to "files-fileList"

[PVince81]

I've reverted the renaming of #permissions as it only ever appears once, so let's not open a can of worms where not needed

then I've renamed #controls to .files-controls and the JS tests all pass now

a bit more manual testing is needed and also more testing with Lighthouse after switching the sections to make sure there is no more duplicate id

[PVince81]

• BUG: grid view in main file list is broken and doesn't appear!

• BUG: ext storage section layout is broken

[PVince81]

• BUG: summary row with "x files" is missing left padding outside of "all files", maybe some CSS selector is not matching

[Pytal]

Seems that global CSS necessitates the use of the files class namespace even in apps that aren't files like comments and systemtags but that is not a concern for this PR

[PVince81]

there's a slight risk that CSS rule priorities will change a bit because targetting a class is weaker than an id.
it happened for the files summary which I just fixed

[PVince81]

thanks for the reviews. I'm going to check the dependent PRs before merging

[PVince81]

• BUG: file picker is broken (seen in Replace id to class for files app text#2745)

it has its own namespace "picker-fileList"

[PVince81]

text app and activity are green

with viewer I'm struggling with CI, so probably I'll skip it and hope it will work with master

[PVince81]

Changes:
* #filestable => .files-filestable
* #fileList => .files-fileList
* #controls => .files-controls
* #emptycontent => .emptyfilelist.emptycontent
* #headerName => .column-name
* #headerDate => .column-mtime
* #headerSize => .column-size

[szaimen]

• find other apps that extend the files app?

Maybe the files_rightclick app?

[PVince81]

Seems that global CSS necessitates the use of the files class namespace even in apps that aren't files like comments and systemtags but that is not a concern for this PR

ideally apps should never rely on those and should actually use the plugin API to attach themselves to the existing classes, like FileList, then they can access this.$table or other sub-elements

[PVince81]

I've grepped the release tarball:

• circles: Rename selectors for files app elements circles#1115

apps/circles/templates/files/list.php
32:    <tbody id="fileList">

apps/circles/js/files/circles.files.list.js
233:                           this.$el.find('#filestable thead th').toggleClass('hidden', this.isEmpty);

apps/circles/js/files/circles.files.list.js
225:                                   this.$el.find('#emptycontent').html('<div class="icon-systemtags"></div>' +
229:                                   this.$el.find('#emptycontent').html('<div class="icon-systemtags"></div>' +
232:                           this.$el.find('#emptycontent').toggleClass('hidden', !this.isEmpty);

nothing else found!