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

Fixes for Breadcrumbs components #1354

Merged
merged 2 commits into from Sep 26, 2015
Merged

Conversation

AlexKVal
Copy link
Member

The continuation of #1273

#1273 (comment) here is my fixes for it
[fixed] Breadcrumb and BreadcrumbItem components (the second commit in this PR)

Looking into it more rigorously:
active property is not needed if stated functionality is implemented

An active class is added to a BreadcrumbItem if there's no href property for it.

Autosetting navItem property onto Breadcrumb children - also is copy/past leftovers.

Both id and linkId have to be string or number types instead just string (to comply with other components).

Notice

This implementation (of BreadcrumbItem) has the same weak spots as some other components have too.

  • className applies only onto wrapper component
  • only limited set of properties, in this case: href, target title it is possible to set for inner anchor element.
  • It is not possible to set custom style or className properties to inner a or span elements.

etc.

It looks now as:
screen shot 2015-09-25 at 9 54 24 pm

@taion
Copy link
Member

taion commented Sep 25, 2015

I really don't like implicitly inferring active for whichever component lacks an href. What if some breadcrumb items just don't have links? I'd be much happier if active were explicit.

@AlexKVal
Copy link
Member Author

np. I'll rewrite it tomorrow.

I've rewritten it to implicitly inferring active because that was in documentation 😄
I'll fix and docs of course.

@AlexKVal
Copy link
Member Author

Done.

Now it looks like:
desc

and props docs
props

/cc @taion

@taion
Copy link
Member

taion commented Sep 26, 2015

LGTM

AlexKVal added a commit that referenced this pull request Sep 26, 2015
Fixes for `Breadcrumbs` components
@AlexKVal AlexKVal merged commit fdae63d into react-bootstrap:master Sep 26, 2015
@AlexKVal AlexKVal deleted the breadcrumbs branch September 26, 2015 15:34
@AlexKVal
Copy link
Member Author

@ycavatars congrats 🎉
screen shot 2015-09-26 at 6 36 51 pm

Thank you for your contribution 🍒

@ycdesu
Copy link
Contributor

ycdesu commented Sep 27, 2015

ya~ thanks for your review! ☀️

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.

None yet

3 participants