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

Isis repaint #11789

Closed
wants to merge 17 commits into from
Closed

Isis repaint #11789

wants to merge 17 commits into from

Conversation

dgrammatiko
Copy link
Contributor

Let's modernise the appearance of isis!

Summary of Changes

A redo of #7610
1 year later and 3rd and last attempt!

This is NOT touching bootstrap LESS files directly, so there shouldn't be any B/C concerns!

Testing Instructions

Apply the patch and visit different areas of admin with isis as the selected template.
This is only a visual update of buttons/dropdowns so watch out for any glitches

Documentation Changes Required

None

Note: although the number of files changed are indicated as 10 the additional ones are only 5, so it's not a great deal in terms of maintenance...

@dgrammatiko
Copy link
Contributor Author

@C-Lodder can you take a look at this one?

@brianteeman
Copy link
Contributor

This will conflict with #11779

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@C-Lodder
Copy link
Member

C-Lodder commented Aug 25, 2016

I was going to say, could you firstly apply Brian's patch please?

Either way, will start looking through this now ;)

@dgrammatiko
Copy link
Contributor Author

@brianteeman it won't as it uses an override for chosen.css that utilises icomoon font instead of the png for the icons.

@brianteeman
Copy link
Contributor

Sorry - didnt spot the different path - but that will mean that you are not
using the correct version of the chosen.css wont it?

On 25 August 2016 at 15:08, Dimitri Grammatikogianni <
notifications@github.com> wrote:

@brianteeman https://github.com/brianteeman it won't as it uses an
override for chosen.css that utilises icomoon font instead of the png for
the icons.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11789 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8fdAoDNv8me1UdnbqG15fy23uBz6ks5qjaHAgaJpZM4JtF-G
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@C-Lodder
Copy link
Member

@DGT41 - Can you ping me a message on Facebook or Skype (charlie.lodder)?

@dgrammatiko
Copy link
Contributor Author

@brianteeman it seems that the css works fine, but honestly I didn't do a diff to your PR to see what else might be changed. Will do that
@C-Lodder sure

@brianteeman
Copy link
Contributor

I am NOT in favour of the changes to remove the button backgrounds as seen on the toolbar and the options.

r3jo

@C-Lodder
Copy link
Member

@DGT41 you've gone offline

@dgrammatiko
Copy link
Contributor Author

@brianteeman I will agree, let me check the css
@C-Lodder that's just an indication 😉

@dgrammatiko
Copy link
Contributor Author

@brianteeman changed it:
screen shot 2016-08-25 at 17 33 13

@brianteeman
Copy link
Contributor

But you havent changed the buttons on the toolbar :p

This also affects buttons used in components like patchtester which also lose their backgrounds

@brianteeman
Copy link
Contributor

Its like a repaint but you forgot to put any paint on the brush

@brianteeman
Copy link
Contributor

Getting there.
I notice on the toolbar buttons which have a colour when you hover and the background goes grey then the icon and the text goes white which is correct

But in some places that doesnt happen eg in the menu item for a single article the select button icon stays green

@brianteeman
Copy link
Contributor

Just a thought
Instead of replacing the current isis why dont we make this a new admin template called something like isis-flat

This one could be the default but it will reduce the "why did you change things" comments if the current one is still available

@C-Lodder
Copy link
Member

@brianteeman - cause it's only the buttons that are being changed. Not really much point in creating another instance of Isis just for this.

@brianteeman
Copy link
Contributor

I missed the edit ;)

@brianteeman
Copy link
Contributor

So the only other thing that looks a little odd to me but dont know if its either just me or even fixable is that when you select an item in a list it goes blue and it stays blue until you do something elsewhere

blue

@dgrammatiko
Copy link
Contributor Author

That's the correct functionality, when you are in a dropdown the focus is on that element, then you select something the dropdown closes but the focus is still on that element. As you said you have to click elsewhere in the page to initiate a blur (loose focus). The only thing that makes this more obvious now is that the dropdown is well visible that is in focus (blue border) which is different from the default chosen css (grey), but IMHO this is more accessible. I can revert the colouring back to grey, but I think this is an improvement

@brianteeman
Copy link
Contributor

As i said - i thought this one was just me thinking it was odd

On 25 August 2016 at 18:50, Dimitri Grammatikogianni <
notifications@github.com> wrote:

That's the correct functionality, when you are in a dropdown the focus is
on that element, then you select something the dropdown closes but the
focus is still on that element. As you said you have to click elsewhere in
the page to initiate a blur (loose focus). The only thing that makes this
more obvious now is that the dropdown is well visible that is in focus
(blue border) which is different from the default chosen css (grey), but
IMHO this is more accessible. I can revert the colouring back to grey, but
I think this is an improvement


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11789 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8S1HDggxSHsurBgEiLMI60mAo8Zgks5qjdXUgaJpZM4JtF-G
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@brianteeman
Copy link
Contributor

@DGT41 not spotted anything else - will keep it as my default for a while and see if I spot anything else

@brianteeman
Copy link
Contributor

Search Tools

See how the icon disappears when the search tools is open but does not have focus

stool

@brianteeman
Copy link
Contributor

The double border issue still exists here in the Single Article menu item options

pinp

@ciar4n
Copy link
Contributor

ciar4n commented Aug 25, 2016

tags

'Close' in tags is slightly out of line. Personally I would be inclined to display such icons inline rather than with absolute positioning. The line-height will center the icon for you rather than the constant tweaking that comes with absolute positioning. Also less CSS.

.chzn-container-multi .chzn-choices .search-choice {
    background-clip: padding-box;
    background-color: #0088cc;
    border-radius: 3px;
    color: #fff;
    cursor: default;
    line-height: 22px;
    margin: 3px 0 3px 5px;
    padding: 0 7px;
}
.chzn-container-multi .chzn-choices .search-choice .search-choice-close {
    color: #f5f5f5;
    display: inline-block;
    margin-left: 5px;
}

@wilsonge wilsonge reopened this Aug 25, 2016
@brianteeman
Copy link
Contributor

Oops sorry I pressed close and comment instead of just comment. Sorry!!!!

@dgrammatiko
Copy link
Contributor Author

@ciar4n thanks!
@brianteeman all fixed now

@brianteeman
Copy link
Contributor

brianteeman commented Aug 25, 2016 via email

@ciar4n
Copy link
Contributor

ciar4n commented Aug 26, 2016

I know I'm splitting hairs here but a subtle border on colored buttons will help make the buttons more unified. Colored next to bordered buttons tend to look a pixel smaller at first glance. A slight border will rectify this?

buttons

.btn-primary {
    background-color: #2384d3;
    border: 1px solid #1c6aa9;
    border: 1px solid rgba(0, 0, 0, 0.25);
    color: #fff;
}
.btn-success {
    background-color: #46a546;
    border: 1px solid #388438;
    border: 1px solid rgba(0, 0, 0, 0.25);
    color: #fff;
}
.btn-danger {
    background-color: #bd362f;
    border: 1px solid #972b26;
    border: 1px solid rgba(0, 0, 0, 0.25);
    color: #fff;
}

.btn:hover, .btn:focus {
    background-color: #808080;
    border: 1px solid #972b26;
    border: 1px solid rgba(0, 0, 0, 0.1);
    color: #fff;
    text-decoration: none;
    text-shadow: none;
}

RGBA colors will translate to :hover. HEX for non RGBA browsers.

@jeckodevelopment
Copy link
Member

Just a question... is this an expected behaviour?
I'm talking about the "grey" pressed button.
buttoninfo

@ciar4n
Copy link
Contributor

ciar4n commented Aug 26, 2016

@jeckodevelopment That is just an example of a hovered item. Sorry I should have stated.

@jeckodevelopment
Copy link
Member

@ciar4n thank you for the explanation :)

@Bakual
Copy link
Contributor

Bakual commented Aug 26, 2016

I'm not a huge fan of this for one reason:

Currently Isis is to a big degree plain Bootstrap, same as Protostar. There are some adjustments in the template.less but that is it.

You are now going to override quite a few files from Bootstrap (and Chosen), making it non-standard.
Since you override the whole file, it's hard to find out how much you changed from the original Bootstrap file. It could be one line or the whole thing.

In the end it means that issues with the original files (be it Bootstrap or Chosen) may go undetected or are hard to test and fix because our default template uses different files.

@mbabker
Copy link
Contributor

mbabker commented Aug 26, 2016

You are now going to override quite a few files from Bootstrap (and Chosen), making it non-standard.

So? By that argument every Bootstrap template should basically look like the starter template. The whole point of using a CSS framework is to give you the toolset you need to create a nice template, not retain its default styling to a damaging extent.

@Bakual
Copy link
Contributor

Bakual commented Aug 26, 2016

Yeah, that's obvious.

I just don't think the default one we ship should override a lot of it. Same as for HTML and JS overrides in our default templates (Protostar and Isis). So far they both use only a few overrides or none. But with this PR that would change.

@mbabker
Copy link
Contributor

mbabker commented Aug 26, 2016

And again, so?

We should be moving to more agnostic default layouts with the framework
stuff in template specific overrides. So I'm less concerned on that aspect.

For JavaScript libraries we should avoid hacking them but there shouldn't
be a rule against extending them for our use cases.

And for CSS, we don't need to be validating the default rules of whatever
we're using. So I'm not concerned with how much (or little) of the core
Bootstrap (or Chosen or whatever else) CSS we use as long as it's all
functional and user friendly.

On Friday, August 26, 2016, Thomas Hunziker notifications@github.com
wrote:

Yeah, that's obvious.

I just don't think the default one we ship should override a lot of it.
Same as for HTML and JS overrides in our default templates (Protostar and
Isis). So far they both use only a few overrides or none. But with this PR
that would change.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11789 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoafoac7HLqhl3Vl8U8xxxvC0RcDqks5qju_BgaJpZM4JtF-G
.

@ciar4n
Copy link
Contributor

ciar4n commented Aug 26, 2016

On the PR... maybe a more uniformed look across all fields. Matching border-radius (3px), border-color (#aaa) and field height (28px). Also the removal of shadows (outset and inset).

Suggested on the right...

fields-uniformed1

*Blue field is :focus

@dgrammatiko
Copy link
Contributor Author

I am closing this in favour of #11832

@dgrammatiko dgrammatiko deleted the isis_again branch March 18, 2017 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants