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

Change padding from 6/7px to 5px #18

Closed
wants to merge 3 commits into
base: 3.x
from

Conversation

Projects
None yet
3 participants
@Skaronator

Skaronator commented May 13, 2017

Its reduce the size of white space between the menu buttons. You'll see more buttons and have less scroll while looking still good.

Here a example:
image

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant May 13, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented May 13, 2017

CLA assistant check
All committers have signed the CLA.

@votdev

This comment has been minimized.

Show comment
Hide comment
@votdev

votdev May 13, 2017

Collaborator

I suggest to use

.x-grid-cell-inner { padding-top: 5px; padding-bottom: 5px; }

because 4px are too small in correlation to the top and bottom toolbars of a grid. The overall look related to the proportions of the UI elements will more fit fit 5px.

Collaborator

votdev commented May 13, 2017

I suggest to use

.x-grid-cell-inner { padding-top: 5px; padding-bottom: 5px; }

because 4px are too small in correlation to the top and bottom toolbars of a grid. The overall look related to the proportions of the UI elements will more fit fit 5px.

@Skaronator

This comment has been minimized.

Show comment
Hide comment
@Skaronator

Skaronator May 13, 2017

Even 3px are fine IMO but going to change it to 5px.

One question: What is the purpose of <div class=" x-tree-elbow-img x-tree-elbow-line" role="presentation"></div>

.x-tree-icon, .x-tree-elbow-img, .x-tree-checkbox those classes have the negative margin of x-grid-cell-inner padding.

Nevermind they are used for the collapse button.

Skaronator commented May 13, 2017

Even 3px are fine IMO but going to change it to 5px.

One question: What is the purpose of <div class=" x-tree-elbow-img x-tree-elbow-line" role="presentation"></div>

.x-tree-icon, .x-tree-elbow-img, .x-tree-checkbox those classes have the negative margin of x-grid-cell-inner padding.

Nevermind they are used for the collapse button.

@Skaronator

This comment has been minimized.

Show comment
Hide comment
@Skaronator

Skaronator May 13, 2017

Updated. A Preprocessors like SASS or LESS might be very helpful to keep consistency over various elements.

Skaronator commented May 13, 2017

Updated. A Preprocessors like SASS or LESS might be very helpful to keep consistency over various elements.

@Skaronator Skaronator changed the title from Change padding from 6/7px to 4px to Change padding from 6/7px to 5px May 13, 2017

@votdev

This comment has been minimized.

Show comment
Hide comment
@votdev

votdev May 13, 2017

Collaborator

Indeed it would be great to use SASS or LESS, but this is not possible (as far as i know) without buying the whole Sencha ExtJS package because this contains the necessary tool chain and files. But i am open for contributions in this behaviour.

Collaborator

votdev commented May 13, 2017

Indeed it would be great to use SASS or LESS, but this is not possible (as far as i know) without buying the whole Sencha ExtJS package because this contains the necessary tool chain and files. But i am open for contributions in this behaviour.

@Skaronator

This comment has been minimized.

Show comment
Hide comment
@Skaronator

Skaronator May 18, 2017

Didn't know that this JS Framework is so limited if you doesn't pay for it. A switch to ReactJS might be good but you won't have the time to do this task.

Anyway I this PR should be ready to merge.

Skaronator commented May 18, 2017

Didn't know that this JS Framework is so limited if you doesn't pay for it. A switch to ReactJS might be good but you won't have the time to do this task.

Anyway I this PR should be ready to merge.

@votdev

This comment has been minimized.

Show comment
Hide comment
@votdev

votdev May 18, 2017

Collaborator

It is not possible nor wanted to switch the UI framework because the code base is to big now. Also ExtJS is one of the best framework, better than AngularJS for example which i am working with, too.

Collaborator

votdev commented May 18, 2017

It is not possible nor wanted to switch the UI framework because the code base is to big now. Also ExtJS is one of the best framework, better than AngularJS for example which i am working with, too.

@votdev

This comment has been minimized.

Show comment
Hide comment
@votdev

votdev May 18, 2017

Collaborator

I have to decline this PR after testing because:

  • the layout of the tree cells is totally shifted and asymetric.
  • the whole UI layout is not fit anymore.

Please use /var/www/openmediavault/css/theme-custom.css to customize the UI to your needs.

Collaborator

votdev commented May 18, 2017

I have to decline this PR after testing because:

  • the layout of the tree cells is totally shifted and asymetric.
  • the whole UI layout is not fit anymore.

Please use /var/www/openmediavault/css/theme-custom.css to customize the UI to your needs.

@votdev votdev closed this May 18, 2017

@Skaronator

This comment has been minimized.

Show comment
Hide comment
@Skaronator

Skaronator May 19, 2017

Weird. Something is wrong with the negative margin. Still don't know why it is there in first place. I've removed it and now its working just fine.

Could you verify that its working now?

Simply put this in the Dev Console when you're on the OMV Interface.

var css = document.createElement("style");
css.type = "text/css";
css.innerHTML = " .x-grid-cell-inner, .x-grid-cell-inner-treecolumn { padding-top: 5px; padding-bottom: 5px;}";
document.head.appendChild(css);

I've updated this in my fork as well but the PR isn't updating: Skaronator@2cbd377

Skaronator commented May 19, 2017

Weird. Something is wrong with the negative margin. Still don't know why it is there in first place. I've removed it and now its working just fine.

Could you verify that its working now?

Simply put this in the Dev Console when you're on the OMV Interface.

var css = document.createElement("style");
css.type = "text/css";
css.innerHTML = " .x-grid-cell-inner, .x-grid-cell-inner-treecolumn { padding-top: 5px; padding-bottom: 5px;}";
document.head.appendChild(css);

I've updated this in my fork as well but the PR isn't updating: Skaronator@2cbd377

@votdev

This comment has been minimized.

Show comment
Hide comment
@votdev

votdev May 19, 2017

Collaborator

Fixed with 5e6b69f.

Collaborator

votdev commented May 19, 2017

Fixed with 5e6b69f.

@Skaronator

This comment has been minimized.

Show comment
Hide comment
@Skaronator

Skaronator commented May 19, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment