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 merge cells style problem when using table substitution #174

Closed
wants to merge 1 commit into from

Conversation

kennycreeper
Copy link

Reproduce The Error

  1. Use table substitution, put the placeholder in a merge cells (B9:C9)
  2. Generate the output
  3. You can see the range C10:C23 border lines doesn't applied

Template
image

Output
image

Resolution

In source code Workbook.prototype.substituteTable function

First declare a mergeCell variable represent the current cell equals to the start cell of some mergeCells
const mergeCell = self.sheet.root.findall("mergeCells/mergeCell").find(c => self.splitRange(c.attrib.ref).start === cell.attrib.r)

Second, if current cell is the start cell, copy the original merge cells, then for loop to append the last of cells in mergeRange

if(mergeCell) {
  var mergeRange    = self.splitRange(mergeCell.attrib.ref),
  mergeStart    = self.splitRef(mergeRange.start),
  mergeEnd      = self.splitRef(mergeRange.end);
  for (let colNum = self.charToNum(mergeStart.col) + 1; colNum <= self.charToNum(mergeEnd.col); colNum++) {
      const lastRow = self.sheet.root.find('sheetData').getItem(mergeStart.row - 1)
      const upperCell = lastRow.getItem(colNum - 1)
  
      const cell = self.cloneElement(upperCell);
      cell.attrib.r = self.joinRef({
        row: newRow.attrib.r,
        col: self.numToChar(colNum)
      });
      newRow.append(cell);
  }
}

@kant2002
Copy link
Collaborator

Thanks for contribution. Can you add test case which validates your scenario, once it is done, I would gladly accept this PR

@@ -1137,6 +1139,23 @@ module.exports = (function() {

// Add the cell that previously held the placeholder
newRow.append(newCell);

if(mergeCell) {
var mergeRange = self.splitRange(mergeCell.attrib.ref),
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you use let instead of var and make each variable as separate variable on individual line.

Copy link
Author

Choose a reason for hiding this comment

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

Ok! I'll work on it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have a chance to work on the suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello,
Thanks for very useful library.
I really need this feature.
Can I know why not merged yet?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@muyoungko can I assume that you take care of this scenario in #188 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes~

@kant2002 kant2002 closed this Mar 11, 2024
@kant2002
Copy link
Collaborator

Thanks for filing this PR. Unfortunately due to long time other contributor come along and provide this fix with tests

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.

3 participants