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

Backend: removed invalid "name" element, removed redundant nobr spans, added data-column-id to grids #3927

Merged
merged 2 commits into from
Apr 8, 2024

Conversation

fballiano
Copy link
Contributor

This PR rewrites #3923

In the process of reviewing #3923 I've seen that the name attribute is not valid for span or a, but the concept of the PR was interesting, so I did a clean up of the whole thing:

  • removed invalid name attribute
  • added data-column-id to the th instead
  • removed the nobr span (there are way too many DOM nodes in OM, they need to be simplified) and added a white-space:nowrap to its container instead

Closes #3923

@fballiano
Copy link
Contributor Author

@loekvangool please check this one instead

@github-actions github-actions bot added Template : admin Relates to admin template Component: Adminhtml Relates to Mage_Adminhtml Component: Widget Relates to Mage_Widget labels Apr 6, 2024
@fballiano fballiano changed the title Backend: remove invalid "name" element, removed redundant nobr spans, added data-column-id to grids Backend: removed invalid "name" element, removed redundant nobr spans, added data-column-id to grids Apr 6, 2024
@loekvangool
Copy link
Contributor

I'm getting aroused! Great stuff

@fballiano
Copy link
Contributor Author

since I've tested this, I'll consider #3927 (comment) as a review and merge it tomorrow if nothing against comes up

@loekvangool
Copy link
Contributor

Okay since you insist I accept my promotion to committer

@@ -114,7 +114,7 @@ $numColumns = count($this->getColumns());
<?php if ($this->getHeadersVisibility()): ?>
<tr class="headings">
<?php foreach ($this->getColumns() as $_column): ?>
<th<?php echo $_column->getHeaderHtmlProperty() ?>><span class="nobr"><?php echo $_column->getHeaderHtml() ?></span></th>
Copy link
Collaborator

@kiatng kiatng Apr 8, 2024

Choose a reason for hiding this comment

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

Not sure about removing the span element. It may break custom styling that selects it, for example:

.grid tr.headings th span.nobr { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is true (I thought about it as I'm very strict with other changes like modifying the signature of a public method) but as the same time the possibility of this happening and the minor side effect made me go for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could merge this in v21 but... it seems overkill for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, I've just removed the .grid tr.headings th span.nobr CSS line which was a hack for firefox 3... firefox 3...

@fballiano fballiano merged commit 91b1944 into OpenMage:main Apr 8, 2024
17 checks passed
@fballiano fballiano deleted the rewrote_pr_3923 branch April 8, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Widget Relates to Mage_Widget Template : admin Relates to admin template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants