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

Some CSS fixes to OpenMage adminhtml theme #2422

Merged
merged 9 commits into from
Sep 5, 2022
Merged

Some CSS fixes to OpenMage adminhtml theme #2422

merged 9 commits into from
Sep 5, 2022

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Aug 14, 2022

Description (*)

  • made columns bit wider
  • removed duplicate code from scss
  • fixed wrong imports in scss
  • added label for OM admin theme

Fixed Issues (if relevant)

  1. Fixes New admin theme: date fields to small #2397

Manual testing scenarios (*)

  1. npm install -g sass
  2. ...

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 environment Template : admin Relates to admin template labels Aug 14, 2022
@sreichel sreichel changed the title Some CSS fixes, fixes #2397 Some CSS fixes Aug 14, 2022
@github-actions

This comment has been minimized.

@sreichel sreichel added Template : admin - openmage Relates to OM admin template and removed Template : admin Relates to admin template labels Aug 14, 2022
@github-actions github-actions bot added the Template : admin Relates to admin template label Aug 14, 2022
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@sreichel sreichel marked this pull request as draft August 14, 2022 05:28
@addison74
Copy link
Contributor

  1. It would be useful if each change was accompanied by a screenshot (for the first and last bullets in the list).

  2. In the Magento theme, I solved the issue in which the date was not fully visible in the input box (PR Backend grid heads - Align the elements for range and date types #2330). Although you made the columns wider in the OpenMage theme, the issue remained. Below a comparison between themes.

data_range

  1. I do not agree that the modified CSS files should be compressed. On the one hand we will get compressed files and others not in the repository and on the other hand, we affect those who were used to uncompressed files in which they made their own changes (not everyone has knowledge of SASS).

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@addison74 addison74 self-assigned this Aug 15, 2022
@addison74
Copy link
Contributor

addison74 commented Aug 15, 2022

I tested the last version and I found that you solved the issue of the visibility of the date selected in the input box.

date

@addison74
Copy link
Contributor

Please analyze whether a change must be made in the case below. The selection should be aligned with the others and have the same width.

price

@addison74
Copy link
Contributor

I appreciate that the CSS files are no longer compressed and it allowed me to go through them in GitHub. You should remove "compressed output" from the PR description.

@addison74
Copy link
Contributor

This comment seems to be too far from the left margin. There are a comments like this one.

distance

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@sreichel sreichel marked this pull request as ready for review August 22, 2022 22:40
@sreichel sreichel marked this pull request as draft August 22, 2022 22:41
@sreichel sreichel marked this pull request as ready for review August 22, 2022 22:44
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@fballiano
Copy link
Contributor

@addison74 do we like this as is it? :-)

@github-actions

This comment has been minimized.

@addison74
Copy link
Contributor

@fballiano - I have to test it.

@github-actions

This comment has been minimized.

@sreichel
Copy link
Contributor Author

@fballiano - I have to test it.

Just changed datefields and currency select fields. For demostore, please enable notice to test.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@addison74
Copy link
Contributor

Please guide me related to these changes:

  • "added label for OM admin theme"
  • "For demostore, please enable notice to test"

@sreichel
Copy link
Contributor Author

First is just a new label for github.

Somewhere in admin config you can enable "Show demostore notice".

@addison74
Copy link
Contributor

I don't have a problem enabling that feature in Backend (Config > General > Design> HTML Head > Display Demo Store Notice = Yes), but let's confirm what are the changes in this PR. At the top there is the notice, the logo on the left and the two elements on the right no longer overlap. Also, if the width of the window is reduced, the elements on the right no longer overlap the menu.

This is before:

before

This is after:

after

Otherwise, they are fine the 2 changes in the grids (date ranges, currency). Also, the CSS files are no longer compressed, which allowed me to follow the changes.

@fballiano
Copy link
Contributor

I'll test this as soon as I close the docblock update, cause I've 3000 modified files in my workspace at the moment :-\

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@addison74 addison74 self-requested a review September 1, 2022 16:59
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

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.

tested, ok

@fballiano fballiano merged commit c691713 into 1.9.4.x Sep 5, 2022
@fballiano fballiano deleted the css-fix branch September 5, 2022 21:25
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 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 c691713. ± Comparison against base commit 028f783.

@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 environment Template : admin - openmage Relates to OM admin template Template : admin Relates to admin template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New admin theme: date fields to small
3 participants