Skip to content

Use certer for vertical-align in Excel when meet vertical-align:middel in CSS #2195

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

Closed

Conversation

nkjackzhang
Copy link

@nkjackzhang nkjackzhang commented Jun 29, 2021

…Excel

This is:

- [x] a bugfix
- [ ] a new feature

Checklist:

Why this change is needed?

When convert HTML to Excel, and meet vertical-align: middle for td element in CSS, we should not set "middle" to vertical field in Alignment, which doesn't work, instead, we should set "center" to it.

@@ -240,6 +240,10 @@ public function setVertical($pValue)
$pValue = self::VERTICAL_BOTTOM;
}

if ($pValue == 'middle') {
Copy link
Member

Choose a reason for hiding this comment

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

It would be better if the method actually validated that the argument passed in was a valid entry from the possible values in the set of defined constants self::VERTICAL_* rather than arbitrary case-sensitive values; providing the default VERTICAL_BOTTOM if the value wasn't valid.

Likewise, it would also be good if a similar check was performed on setting horizontal alignment

Copy link
Author

Choose a reason for hiding this comment

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

I find a function in Writer/Html, which is used to map vertical alignment in Excel to vertical-align in CSS, but it maps both center and justify in Excel to middle in CSS. Can i only map middle to certer and ignore justify?

Copy link
Member

Choose a reason for hiding this comment

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

The solution then should be in the mapping for the HTML Writer, providing a mechanism that allows the defined valign mapping VALIGN_ARR to be overridden with a custom mapping

oleibman added a commit to oleibman/PhpSpreadsheet that referenced this pull request Sep 5, 2022
This PR expands on PR PHPOffice#2195 from @nkjackzhang. That PR has been stalled for some time awaiting requested fixes. Those fixes are part of this PR, and additional tests and samples are added. The original request was to handle `vertical-align:middle` in Css (Excel uses `center`). This PR does its best to also handle vertical alignment Excel values not found in Css - `justify` (as `middle`) and `distributed` (as `middle`). It likewises handles valid Css values not found in Excel (`baseline`, `sub`, and `text-bottom` as `bottom`; `super` and `text-top` as `top`; `middle` as `center`).

It also handles horizontal alignment Excel values not found in Css - `center-continuous` as `center` and `distributed` as `justify`; I couldn't think of a reasonable equivalent for `fill`, so it is ignored.

The values assigned for vertical and horizontal alignment are now lower-cased (special handling required for `centerContinuous`).
@oleibman oleibman marked this pull request as draft September 5, 2022 04:37
@oleibman
Copy link
Collaborator

oleibman commented Sep 5, 2022

Converting to draft. Change is superseded by PR #3048. If that change is merged, I will close this one.

oleibman added a commit that referenced this pull request Sep 9, 2022
This PR expands on PR #2195 from @nkjackzhang. That PR has been stalled for some time awaiting requested fixes. Those fixes are part of this PR, and additional tests and samples are added. The original request was to handle `vertical-align:middle` in Css (Excel uses `center`). This PR does its best to also handle vertical alignment Excel values not found in Css - `justify` (as `middle`) and `distributed` (as `middle`). It likewises handles valid Css values not found in Excel (`baseline`, `sub`, and `text-bottom` as `bottom`; `super` and `text-top` as `top`; `middle` as `center`).

It also handles horizontal alignment Excel values not found in Css - `center-continuous` as `center` and `distributed` as `justify`; I couldn't think of a reasonable equivalent for `fill`, so it is ignored.

The values assigned for vertical and horizontal alignment are now lower-cased (special handling required for `centerContinuous`).
@oleibman
Copy link
Collaborator

oleibman commented Sep 9, 2022

Superseded by 3048, which has just been merged. Closing this PR.

@oleibman oleibman closed this Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants