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

FLUID-6298: Updating and expanding contrast themes. #938

Merged
merged 13 commits into from Oct 30, 2018

Conversation

Projects
None yet
3 participants
@jobara
Copy link
Member

commented Oct 10, 2018

  • Added a grey on dark theme
  • expanded grey on white and black on brown themes to include additional
    colours for links, buttons, and selection. This is to match Windows
    Contrast themes.

https://issues.fluidproject.org/browse/FLUID-6298

FLUID-6298: Updating and expanding contrast themes.
- Added a grey on dark theme
- expanded grey on white and black on brown themes to include additional 
colours for links, buttons, and selection. This is to match Windows 
Contrast themes.
@jobara

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

@jhung could you please take a look at this PR? There are a lot of Styling changes.

@incd-ci-robot

This comment has been minimized.

@simonbates simonbates self-requested a review Oct 16, 2018

@incd-ci-robot

This comment has been minimized.

@incd-ci-robot

This comment has been minimized.

@incd-ci-robot

This comment has been minimized.

@incd-ci-robot

This comment has been minimized.

}
}

if (disabledColor) {

This comment has been minimized.

Copy link
@simonbates

simonbates Oct 26, 2018

Member

Minor thing, but I think I would swap the order here of the if (selectedFColor... and if (disabledColor... blocks so that they match the order of specification of the values above beginning line 136.

@@ -301,17 +350,44 @@ build-themes-Enactors(contrastThemes) {
border-left-color: bColor !important;
}

if (selectedFColor && selectedBColor) {

This comment has been minimized.

Copy link
@simonbates

simonbates Oct 26, 2018

Member

Would we also want an entry in the .fl-inverted-color block for disabledColor?

This comment has been minimized.

Copy link
@jobara

jobara Oct 27, 2018

Author Member

My initial thought is that it isn't needed, but I'm open to changing things. If I remember correctly the fl-inverted-color class was provided as a means to make certain portions of the content to stick out. This is particularly necessary for our binary colour schemes. In the prefs framework demo we use this for a header. If you were to have a whole section marked as fl-inverted-color, which I can't think of a reason for at the moment, I think you'd still want the disabled inputs to look the same for consistency.

Now that i'm thinking more about this, i'm not sure that it's necessary to invert the selection styles either.

Please let me know what you think about these.

<div class="fl-inverted-color">
<div class="left_tab"></div>
<div class="middle_tab">
<h2 class="h1_middle_text fl-inverted-color">Resources</h2>

This comment has been minimized.

Copy link
@simonbates

simonbates Oct 26, 2018

Member

With the new fl-inverted-color container div, did we also want to keep the fl-inverted-color class here on the h2? We now have that class in 2 places: on a container and on the element.

@incd-ci-robot

This comment has been minimized.

@@ -291,6 +291,119 @@ <h4>Mid-term Exam</h4>
<textarea>Please enter a simple JavaScript function to find the smallest value of an array.</textarea>
<button>Run Script</button>
</fieldset>
<fieldset class="disabled-inputs">
<legend>HTML Property</legend>

This comment has been minimized.

Copy link
@simonbates

simonbates Oct 29, 2018

Member

I think that we are either missing a layer of fieldset here or we need to change this legend. At the moment it looks like the whole disabled inputs fieldset is called "HTML Property" rather than something like "Disabled Inputs".

@incd-ci-robot

This comment has been minimized.

@incd-ci-robot

This comment has been minimized.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2018

@simonbates I've addressed the issues we talked about earlier today. This PR is ready for more review.

@simonbates simonbates merged commit e34013b into fluid-project:master Oct 30, 2018

2 checks passed

jenkins Build finished.
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.