Skip to content
This repository was archived by the owner on Oct 8, 2021. It is now read-only.

Conversation

@cogwurx
Copy link
Contributor

@cogwurx cogwurx commented Feb 22, 2016

Added a bit more contrast to the hover states for the Navigation Panel/Accordion as well as the Search Results Panel.
Darkened the hover state of text links to also give more contrast and make obvious that the link is a link.

@jquerybot
Copy link

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

opacity: .75;
}

/* Darker hover state for links. Overwriting jquery.mobile.theme.css LN298 */
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove line number (theme CSS isn't static so the line number will change sooner or later).

@jaspermdegroot
Copy link
Contributor

@cogwurx

The changes look good. I noticed one problem. When an accordion is being expanded it gets the green active style, but this style is not removed after the accordion is being collapsed. It remains green until another button gets focus. In the current situation the active style isn't really clear, but it does get removed as soon as the accordion is collapsed (http://view.jquerymobile.com/1.5-demos-issues/demos/). Can you give this a look?

Besides that there are a few minor style guide issues. See my comments in the diff.

Thanks!

@cogwurx
Copy link
Contributor Author

cogwurx commented Mar 9, 2016

@jaspermdegroot

Just pushed the updates.Fixed the style guide spacing and the accordion focus.Let me know if there is anything else.

.jqm-navmenu-panel > .ui-panel-inner > .ui-listview li > .ui-listview-item-button:focus,
.jqm-navmenu-panel > .ui-panel-inner > .ui-listview li > .ui-listview-item-button:active,
.jqm-navmenu-panel > .ui-panel-inner > .ui-listview li > .ui-accordion-header:hover,
.jqm-navmenu-panel > .ui-panel-inner > .ui-listview li > .ui-accordion-header:active,
Copy link
Contributor

Choose a reason for hiding this comment

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

.jqm-navmenu-panel > .ui-panel-inner > .ui-listview li > .ui-accordion-header:focus, is missing here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I removed that due to the focus having a dark gray background style remaining on the accordion header after closure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, that's because it has focus after closing. If you now tab key navigate through the menu, buttons that are links get focus style but buttons that are accordion headers don't. That's bad.

@jaspermdegroot
Copy link
Contributor

@cogwurx

Thanks. See my two comments on the diff. After that it's good to land.

@arschmitz
Copy link
Contributor

landed this with other demos issues on 1.5-dev

@arschmitz arschmitz closed this Mar 26, 2016
@jaspermdegroot
Copy link
Contributor

This wasn't ready to land. We still have to add .jqm-navmenu-panel > .ui-panel-inner > .ui-listview li > .ui-accordion-header:focus back.

jaspermdegroot pushed a commit that referenced this pull request Apr 1, 2016
arschmitz pushed a commit to arschmitz/jquery-mobile that referenced this pull request Jul 4, 2016
arschmitz pushed a commit that referenced this pull request Jul 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants