Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

fix(list): show item content separated to the button #6493

Closed

Conversation

devversion
Copy link
Member

Fixes #6984 Fixes #6488

@EladBezalel EladBezalel added the needs: manual testing This issue or PR needs to have some manual testing and verification done label Dec 31, 2015
@@ -5,7 +5,7 @@ a.md-button.md-THEME_NAME-theme,
&:hover {
background-color: '{{background-500-0.2}}';
}
&.md-focused {
&.md-focused:not(.md-no-focus) {
Copy link
Member

Choose a reason for hiding this comment

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

it would be so much clearer if you would just wrap all with

&:not(.md-no-focus) 

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not getting what you exactly mean. You mean we should wrap that as followed:

    &.md-focused {
      &:not(md-no-focus) {
        background-color: '{{background-500-0.2}}';

      }
    }

Because I think this will add an empty selector, which is not used. (only &.md-focused)

Copy link
Member

Choose a reason for hiding this comment

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

NVM but i just feel like this is wrong, can't we apply md-focused only when md-no-focus is not available?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to be a better a idea, I would prefer a attribute called md-no-focus-style. See my newest changes, I updated my PR, you should decide about className or attribute

@EladBezalel
Copy link
Member

@devversion in the issue the user claims he can't use 2 checkboxes what is your fix right here?

@EladBezalel EladBezalel added needs: more info The issue does not contain enough information for the team to determine if it is a real bug and removed needs: manual testing This issue or PR needs to have some manual testing and verification done labels Dec 31, 2015
@devversion
Copy link
Member Author

I completely reworked the list item structure, in this version we split the button and the content, thats really important because firefox is not allowing by default click events on children. This fix works as same as before, but it only fixes the issues with firefox and other browsers which disallow click events on child nodes

@devversion
Copy link
Member Author

As you want receive more info, what info is missing?. You can see the Firefox Bug Report on Bugzilla too, https://bugzilla.mozilla.org/show_bug.cgi?id=1181763

@EladBezalel
Copy link
Member

The code looks much clearer right now.

@EladBezalel EladBezalel added needs: review This PR is waiting on review from the team and removed needs: more info The issue does not contain enough information for the team to determine if it is a real bug labels Jan 4, 2016
@devversion
Copy link
Member Author

@ThomasBurleson @EladBezalel I just rebased the PR (due conflicts from 3ffa0a2) and then I combined it with a fix for #6984.

Tested it, this will fix the click issues for firefox (https://bugzilla.mozilla.org/show_bug.cgi?id=1181763) and will fix the current button / secondary issue (disconnected button from item content)

Here a preview after the fix:

@ThomasBurleson ThomasBurleson added pr: merge ready This PR is ready for a caretaker to review and removed needs: review This PR is waiting on review from the team labels Feb 4, 2016
@ThomasBurleson ThomasBurleson added this to the 1.0.5 milestone Feb 4, 2016
ErinCoughlan pushed a commit to ErinCoughlan/material that referenced this pull request Feb 9, 2016
@AnirudhaGohokar
Copy link

Hi,
Was applying md-seconday to md-input-container but it does'nt seem to work.

http://codepen.io/anon/pen/pgmwBv

I believe its for only buttons but what if we want to have non clickable contents like input control ?

Thank You

@devversion
Copy link
Member Author

@AnirudhaGohokar,

Yes you're right, as the docs say, it's only restricted for use with buttons. If you would suggest this a feature, you should create another issue.

@devversion devversion deleted the fix/list-content-seperate branch April 19, 2016 19:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants