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: revert to old behavior but properly generate picker labels #1606

Closed

Conversation

zewa666
Copy link
Contributor

@zewa666 zewa666 commented Jul 15, 2024

fixes #1605

Copy link

stackblitz bot commented Jul 15, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

codecov bot commented Jul 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.8%. Comparing base (f03cc6c) to head (e68bc6b).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1606     +/-   ##
========================================
- Coverage    99.8%   99.8%   -0.0%     
========================================
  Files         198     198             
  Lines       21795   21794      -1     
  Branches     7303    7159    -144     
========================================
- Hits        21734   21733      -1     
+ Misses         61      55      -6     
- Partials        0       6      +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -593,8 +593,7 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
if (emptyTarget) {
emptyElement(target);
}
const node = options?.cloneNode ? val.cloneNode(true) : val;
Copy link
Owner

Choose a reason for hiding this comment

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

could you also remove the options prop above cloneNode?: boolean; since it's no longer needed

if (column?.name instanceof HTMLElement || column?.name instanceof DocumentFragment) {
return column.name.textContent ?? '';
}

Copy link
Owner

@ghiscoding ghiscoding Jul 15, 2024

Choose a reason for hiding this comment

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

so I had a very similar approach but I think it would be better to go with mine instead, because with yours, you forgot to deal with the line 280. So I think it's better to first extract the column name for all other usage as shown below

Suggested change
function pickerHeaderColumnValueExtractor(column: Column, gridOptions?: GridOption) {
let colName = column?.name ?? '';
if (colName instanceof HTMLElement || colName instanceof DocumentFragment) {
colName = colName.textContent || '';
}
const headerGroup = column?.columnGroup || '';
const columnGroupSeparator = gridOptions?.columnGroupSeparator ?? ' - ';
if (headerGroup) {
return headerGroup + columnGroupSeparator + colName;
}
return colName;
}

@zewa666 zewa666 closed this Jul 15, 2024
@ghiscoding
Copy link
Owner

ghiscoding commented Jul 15, 2024

@zewa666 oh I'm not sure why you closed your PR?

I had another possible problem with the .textContent, this will read all element texts but is that really ideal though? I mean I just tested this with an HTML element that has some text and a button with text and it gives me what is shown below (the button text is also pulled). However, I'm not sure that we can do much about it?!?

image

that is using the following code

const div = document.createElement('div');
    const button = document.createElement('button');
    button.className = 'button button-small';
    button.textContent = 'click';
    button.addEventListener('click', () => alert('hi'));
    div.appendChild(document.createTextNode('Column 1 '));
    div.appendChild(button);

    this.columnDefinitions = [
      {
        id: 'sel', name: div, field: 'num', width: 80, type: FieldType.number,
        excludeFromExport: true,
        resizable: true,
        filterable: true,
        selectable: false,
        focusable: false
      },

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

custom HTMLElement inside header loses event handler
2 participants