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

Set default width for grid datetime columns, ref #2239 #2544

Merged
merged 3 commits into from
Sep 6, 2022
Merged

Set default width for grid datetime columns, ref #2239 #2544

merged 3 commits into from
Sep 6, 2022

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Sep 3, 2022

Description (*)

This PR replaces all admin grid width attributes for datetime columns width a default value to make grids more consistent.

The required width for datetime is allways same, so it does not make sense to set it 140 here, to 200 there ...

This could be extended to set defaults for other grid types too (date, action, currency, store ...)

Fixed Issues (if relevant)

  1. Fixes Inconsistent column width in Backend grids. #2239

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Index Relates to Mage_Index Component: Sales Relates to Mage_Sales labels Sep 3, 2022
@sreichel sreichel changed the title [WIP] Set default width for admin grid datetime columns, ref #2239 Set default width for admin grid datetime columns, ref #2239 Sep 3, 2022
@sreichel sreichel changed the title Set default width for admin grid datetime columns, ref #2239 Set default width for grid datetime columns, ref #2239 Sep 3, 2022
@sreichel sreichel requested a review from addison74 September 3, 2022 03:44
@sreichel sreichel marked this pull request as draft September 3, 2022 06:34
@addison74
Copy link
Contributor

The inconstant width of the columns in the grids is a disturbing visual issue in the Backend and which I reported here #2239. There are over 120 Grid.php files and this issue should be solved in the following order:

  • default values for all types of columns, exactly as proposed in this PR. such an implementation no longer requires changes in the boxes.css file as I evaluated in my reported issue.
  • establishing a width value in the Grid.php file for a column only where necessary, for example the name of the products, buyer names.

I'm testing and if it's fine it can be proposed for approval.

@sreichel sreichel marked this pull request as ready for review September 4, 2022 23:40
@addison74
Copy link
Contributor

This PR must be tested together with PR #2422. The latter fixes the issues related to the date columns in the grids. Yesterday it took me almost half an hour to understand why only by taking this PR further the columns were not presented correctly, the calendar icon moving to the next line.

fballiano
fballiano previously approved these changes Sep 5, 2022
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

LGTM

@addison74
Copy link
Contributor

addison74 commented Sep 5, 2022

This PR is fine. Two comments:

  1. the value for the default width should be changed from 140 to 150. I showed above with images the reason for the change.
  2. we need to check if the width have been deleted in all Grid.php files, where the type is "date" and "datetime". I haven't checked how many there are, they seem a little around 20.

@addison74
Copy link
Contributor

If the OpenMage theme is used then the width value 150 is not suitable, but 155. Below the difference between 150 and 155.

openmage_theme_before

openmage_theme

@addison74
Copy link
Contributor

I checked all the changes and they are fine in the Grid.php files:

type => 'data' - 17 matches in 10 files
type => 'datetime' - 27 matches in 19 files

In order to approve it, I am waiting for the opinion related to the value of the default width at 150/155.

@sreichel
Copy link
Contributor Author

sreichel commented Sep 5, 2022

I've changed datetime column width to 160px (as in first commit).

Note: there is (at least) one grid that is still "broken", because all columns have width set. I cant find it right now and will be fixed later.

@sreichel
Copy link
Contributor Author

sreichel commented Sep 6, 2022

@addison74 just an idea ... some date columns are aligned left, others centered. Centered looks better imho ...

@addison74
Copy link
Contributor

I will tell you my opinion in a discussion post. Left and right alignment is prefered for most columns. Center align for a few, for example Action column or colums with words closed as length (Enable/Disable).

'width' => 140
],
];

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

The column looks like this with 140:

screenshot_before

And it looks like this with 150:

screenshot_after

As can be seen at the value of 150, it is on a single line. I propose changing the value from 140 to 150.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is 150 enought when the hour is like “22.00”? Cause there’s gonna be ine more digit.

now that @sreichel set it to 160 i think its ok

Copy link
Contributor

Choose a reason for hiding this comment

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

160 seems to cover all scenarios. If not we can create a new PR later. We should implement default values for other types (TBD).

@addison74
Copy link
Contributor

I have not checked any columns of type date. Maybe the change from 140 to 160 should be made there as well.

@addison74 addison74 self-assigned this Sep 6, 2022
@sreichel
Copy link
Contributor Author

sreichel commented Sep 6, 2022

No. Date columns require less space (no time shown) and could be even smaller, but column header needs around 140px.

@addison74
Copy link
Contributor

In boxes.css (Magento legacy theme) and override.css (OpenMage theme) there is already established the minimum width value at 140px. Practically, through this PR we control a value greater than this for all grids which is useful in my opinion. It can be seen from the modified files that there is an inconsistency related to the value of the two columns, one of which even has a value of 1.

@sreichel sreichel merged commit 0c2fb34 into OpenMage:1.9.4.x Sep 6, 2022
@sreichel sreichel deleted the admin-grid branch September 6, 2022 20:17
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 0c2fb34. ± Comparison against base commit c691713.

@addison74 addison74 removed their assignment Jul 14, 2023
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: Index Relates to Mage_Index Component: Sales Relates to Mage_Sales
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent column width in Backend grids.
3 participants