-
Notifications
You must be signed in to change notification settings - Fork 26
feat(a11y-table): Define a standalone table component #1634
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
base: master
Are you sure you want to change the base?
Conversation
596aec9
to
972eb6a
Compare
canvastablecontainer { | ||
opacity: 0; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better to instead get rid of anything canvas related in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Remove this.canv and the canvas element from canvastable
src/app/app.component.scss
Outdated
@@ -68,3 +72,10 @@ | |||
width: 150px; | |||
} | |||
|
|||
.text-primary { | |||
color: #01579b; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to a css property
Initial thoughts
|
src/app/app.component.ts
Outdated
async enrichRows() { | ||
const { start, end } = this.renderedRange | ||
|
||
this.rows = await Promise.all(mapOverIndexes(async (value) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might cause a race condition. When renderedRange changes twice; it might happen that the first range change resolves later than the second range change.
- Find a way to merge resolved arrays in a way that will keep the latest and not overwrite fetched rows
7c02014
to
62b6d40
Compare
@@ -0,0 +1,13 @@ | |||
<cdk-virtual-scroll-viewport [itemSize]="firstRowHeight" class="email-viewport"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Rename to virtual-scroll-table.component
import { debounceTime } from 'rxjs/operators'; | ||
|
||
@Component({ | ||
selector: 'app-accessible-table', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Rename to
app-virtual-scroll-table
src/app/app.component.html
Outdated
<td class="checkbox-cell"> | ||
<mat-checkbox | ||
(click)="onCheckboxClick($event, item, index)" | ||
[checked]="rowsSelectionModel.isSelected(item)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Remove event listener from the skeleton version of a row.
src/app/app.component.html
Outdated
(sortToggled)="updateSearch(true)"> | ||
</canvastablecontainer> | ||
|
||
<app-accessible-table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@castaway , at some point I think it makes sense to move this table definition into its own component.
I believe that time to be when we want to re-use this table in other places either because we want to have better routing for folders or for some other reason that requires re-use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its that sizable that I'd suggest earlier rather than later (since app.component.html is chunky already)
src/app/app.component.scss
Outdated
|
||
} | ||
|
||
.skeleton-bone { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move skeleton styles to global styles.
src/app/human-bytes.pipe.ts
Outdated
|
||
const result = (value / Math.pow(base, exponent)).toFixed(decimalPlaces); | ||
return `${parseFloat(result)} ${suffixes[exponent]}`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears we have a function in src/app/messagetable/messagetablerow.ts
. The converting of a number that represents the amount of bytes to a human readable format is not specific to messagetable.
- Make a human-readable utility module and use that here and in the messagetablerow.ts.
const selection = (this.selectionModel.isMultipleSelection() ? items : [items]) as T[]; | ||
this.selectionModel.setSelection(...selection) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Angular allows for two way binding in the template. Here we wrap the angular Selection model and add the selected property to be used for binding.
} | ||
}); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A SelectionModel that takes a predicate which is used to filter items selected using the .select(...items) method. This is handy to prevent selecting items that have not been loaded yet. This is relevant when doing a range select which contains items that have not been loaded into array yet.
@HostListener('window:resize') | ||
onWindowResize() { | ||
this.resetWidth(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be here? When do we want to reset the widths?
size: this.getRow(rowIndex).size, | ||
}; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@castaway , could you please help with how to test the websocketsearchmaillist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
websocketsearch is used when:
- The index sync is turned off
- A search is made (this hits an API which returns search results from the index on the server)
} catch (e) { | ||
// This shouldnt happen, it means something changed the stored | ||
// data without updating the messagedisplay rows. | ||
console.log('Tried to lookup ' + index + ' in searchIndex, isnt there! ' + e); | ||
console.error('Tried to lookup ' + index + ' in searchIndex, isnt there! ' + e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Change
+ e
to, e
for more useful log in console
seen: this.searchService.getDocData(this.getRowId(index)).seen, | ||
}; | ||
|
||
if (app.viewmode === 'conversations') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Add the conversation count also to the accessible table as it was in canvastable.
src/app/xapian/searchservice.ts
Outdated
// console.log(`${f}`); | ||
// console.log(FS.stat(`${this.partitionsdir}/${f}`)); | ||
}); | ||
// }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the look as it does nothing but block the thread. Easy performance win
Notes while testing/poking around - mostly comparing to existing prod (since we didnt write a spec..)
|
Starting new comment since that one seems full: |
fef59c7
to
67fd747
Compare
44e5ea1
to
92576fa
Compare
@castaway , I am unable to reproduce this on my account. I could do a null check and drop the message when it is null. |
36c5e72
to
c38c2fe
Compare
871291d
to
ef1226e
Compare
27a5552
to
108bd66
Compare
6c96136
to
f57a331
Compare
99a1649
to
aff3997
Compare
aff3997
to
c45faa0
Compare
- Improves on the less accessible canvastable. - Introduces multi select using ctrl and shift. - Reduces memory footprint by a good margin.
c45faa0
to
3fb375b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some queries/changes
src/app/app.component.html
Outdated
(sortToggled)="updateSearch(true)"> | ||
</canvastablecontainer> | ||
|
||
<app-accessible-table |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its that sizable that I'd suggest earlier rather than later (since app.component.html is chunky already)
@@ -32,7 +32,7 @@ export class SearchMessageDisplay extends MessageDisplay { | |||
} | |||
|
|||
getRowSeen(index: number): boolean { | |||
return this.searchService.getDocData(this.rows[index][0]).seen ? false : true; | |||
return this.searchService.getDocData(this.getRowId(index)).seen; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused how / why, this state of this boolean (it was "if seen then return false" .. yes I know its weird), has changed, but the same method in messagelist.ts didnt.. so now the two are returning different states for seen yes/no ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll just revert it to what it was
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment for all the new components: The existing code tends to have as a major structure, top level directories are areas of the application, canvastable, mailviewer etc (more or less), and the components for an area go inside that directory - or in common/ if they are used by multiple areas .. so could we move the new components into sub-directories appropriately please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app.component is indeed very big. We should have more routes defined which would result in a smaller app.component. I suggest that instead of wrapping it in a component which requires passing loads of @Inputs
or imports.
size: this.getRow(rowIndex).size, | ||
}; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
websocketsearch is used when:
- The index sync is turned off
- A search is made (this hits an API which returns search results from the index on the server)
…nt virtual scroll table
…Implement virtual scroll table
…list): Implement virtual scroll table
} | ||
|
||
updateRows() { | ||
this.rows = this.canvastable?.rows?.rows ? [...this.canvastable.rows.rows] : [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is "fetch rows we just stored in canvastable, back out again", then it would be less complex to change setMessageDisplay to write to this.rows directly
…essage-list): Implement virtual scroll table
… feat(message-list): Implement virtual scroll table
… fixup! feat(message-list): Implement virtual scroll table
}; | ||
// Send to the messageCache in the worker, so we can add the text to the index: | ||
if(updateWorker.size > 0) { | ||
this.searchService.indexWorker.postMessage({'action': PostMessageAction.messageCache, 'updates': updateWorker }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this happen now? (I can't find a replacement for this code)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what I broke by removing this. I'll add it to the todo of this PR.
Canvastable use review: Summary:
Raw NOTES:
|
Ah just noticed this part too:
|
Add table to the app component.
Use j and k to scroll up and down in messages.Dropped the support for j and k scroll.$
postfix for rxjs stream variables