Skip to content

Conversation

tannewt
Copy link
Contributor

@tannewt tannewt commented Sep 6, 2015

No description provided.

@jquense
Copy link
Member

jquense commented Sep 7, 2015

is this a feature from bootstrap? could you provide more context please?

@tannewt
Copy link
Contributor Author

tannewt commented Sep 8, 2015

I think you could say that. This change makes the inner text of the dropdown match this less rule from Bootstrap: https://github.com/twbs/bootstrap/blob/master/less/navbar.less#L434

@taion
Copy link
Member

taion commented Sep 11, 2015

Does this properly prevent the Dropdown button from being clicked?

@tannewt
Copy link
Contributor Author

tannewt commented Sep 11, 2015

The click is already correctly prevented. This change only affects the appearance so that it looks disabled.

@taion
Copy link
Member

taion commented Sep 11, 2015

Ah. Cool. LGTM. Need +1 reviewer.

@jquense
Copy link
Member

jquense commented Sep 11, 2015

I am a bit concerned that this is too implementation specific to go on the general Dropdown. would this present annoying styling resets for custom menus/toggles? Would it make more sense for the DropdownButton(s) to set it?

@taion
Copy link
Member

taion commented Sep 11, 2015

Wouldn't that only be the case if they've defined a .disabled rule that would apply? Maybe I'm misunderstanding.

@taion
Copy link
Member

taion commented Sep 25, 2015

@jquense ping

@AlexKVal
Copy link
Member

Actually this is the fix for undiscovered, till now, bug.
@tannewt could you please add [fixed] into the beginning of your commit header ?
(it is then will be reflected in the Changelog)

Without this fix:
not-fixed

And with this fix:
fixed

and
fixed-neg

@tannewt
Copy link
Contributor Author

tannewt commented Sep 26, 2015

@AlexKVal Done.

@AlexKVal
Copy link
Member

I presume we cannot merge it yet because of @jquense concern.
Will wait his comments on this.
🍒

@jquense
Copy link
Member

jquense commented Sep 27, 2015

I think my concern as abated for now. LGTM

taion added a commit that referenced this pull request Sep 27, 2015
Set the disabled css class so that the text is greyed out.
@taion taion merged commit 9898f4c into react-bootstrap:master Sep 27, 2015
@AlexKVal
Copy link
Member

@tannewt thank you for your contribution 🍒

@tannewt
Copy link
Contributor Author

tannewt commented Sep 28, 2015

Np! Happy to help.
On Mon, Sep 28, 2015 at 2:21 AM Alexander Shemetovsky <
notifications@github.com> wrote:

@tannewt https://github.com/tannewt thank you for your contribution [image:
🍒]


Reply to this email directly or view it on GitHub
#1295 (comment)
.

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.

4 participants