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

Conversation

@chrisdemars
Copy link
Contributor

I added alternative text to the GitHub logo in the header and the combineAll graphic in the operators section.

Added alternative text to the GitHub logo in the header.

n/a
Added alternative text to the combineAll image.

n/a
@ladyleet
Copy link
Member

@chrisdemars woohoo your first PR here! congrats!

@chrisdemars
Copy link
Contributor Author

Stoked to get the first one in @ladyleet! 🔥

@ladyleet
Copy link
Member

@chrisdemars small little conflicts with your branch - can you resolve?

@chrisdemars
Copy link
Contributor Author

Sure @ladyleet

@chrisdemars
Copy link
Contributor Author

I actually don't know which version is the correct one @ladyleet 🙁

@chrisdemars
Copy link
Contributor Author

With help from @knittingcodemonkey, I think the merge is fixed.

@ladyleet
Copy link
Member

great! need one more reviewer and someone to push! :) i've already approved.

Copy link
Contributor

@knitcodemonkey knitcodemonkey left a comment

Choose a reason for hiding this comment

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

The update seems to have resolved the merge conflict

[src]="url"
*ngIf="url" />
*ngIf="url"
alt="Diagram of how combineAll works" />
Copy link
Collaborator

@btroncone btroncone Dec 12, 2017

Choose a reason for hiding this comment

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

I think we need to make a small update so combineAll isn't hardcoded, something like:

alt="Diagram of how {{operatorName}} works"

Or we can add a getter to this component for the alt text:

get altText() {
  return `Diagram of how ${this.operatorName} works`;
}

then:

[alt]="altText"

Thanks a ton for your help!

…mage.

Added JS to generate the alt text for the combineAll image.

n/a
@chrisdemars
Copy link
Contributor Author

I believe I made the change that @btroncone suggested.

Copy link
Collaborator

@btroncone btroncone left a comment

Choose a reason for hiding this comment

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

Thanks! lgtm 👍

@sumitarora sumitarora closed this Dec 13, 2017
@sumitarora sumitarora reopened this Dec 13, 2017
@chrisdemars
Copy link
Contributor Author

I think this might be ready to be merged? Possibly?

@btroncone
Copy link
Collaborator

Ready to merge this but looks like TravisCI is blocking?

@benlesh benlesh reopened this Dec 16, 2017
@benlesh benlesh merged commit 9f7b9df into ReactiveX:master Dec 16, 2017
@benlesh
Copy link
Member

benlesh commented Dec 16, 2017

Admin squash and merge because Travis was not reporting back at all and didn't appear to know about the build even.

@chrisdemars
Copy link
Contributor Author

Thanks @benlesh and @knittingcodemonkey

@benlesh
Copy link
Member

benlesh commented Dec 16, 2017

Thank YOU!

@chrisdemars chrisdemars deleted the chrisdemars-img-alt branch December 16, 2017 02:24
@ladyleet
Copy link
Member

@chrisdemars srsly thank you so much for contributing! glad we finally got this in. :) and thank you @benlesh!

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.

7 participants