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

Add language icon to team stream #3072

Closed

Conversation

bradhvr
Copy link

@bradhvr bradhvr commented Aug 20, 2016

I added the language icon to the top right of the team stream solutions, per issue #3020.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.667% when pulling 0bde320 on bhoover85:add_languange_icon_to_team_stream into 8bbe89d on exercism:master.

@Insti
Copy link

Insti commented Aug 21, 2016

Great ❤️ Can you add before/after screenshots to this PR please.
(I know you've done some in the linked issue but it makes evaluating the PR easier if the changes are clearly visible here when reviewing.)

@bradhvr
Copy link
Author

bradhvr commented Aug 21, 2016

Sure, here you go...

Before
2016-08-21_11h46_18

After
2016-08-21_11h45_43

@Insti
Copy link

Insti commented Aug 21, 2016

Will this also cause the icon to appear on regular language streams?
In which case all the language icons will be the same.

(It's probably fine if it does, but it would be nice to have the consideration documented and maybe add a screenshot of that case too.)

@bradhvr
Copy link
Author

bradhvr commented Aug 21, 2016

You mean if someone filters by one of the languages? Yes it will appear on those as well.

2016-08-21_12h33_44

@Insti
Copy link

Insti commented Aug 21, 2016

Also on the regular language tracks that aren't part of a team stream, for example: http://exercism.io/tracks/ruby/exercises

@bradhvr
Copy link
Author

bradhvr commented Aug 21, 2016

Oh I see... yes it does.

2016-08-21_12h53_55

@Insti Insti added the ready label Aug 21, 2016
@Insti Insti mentioned this pull request Aug 21, 2016
@@ -17,6 +17,7 @@
<h4>
<%= exercise.problem.name %>
<span class="profile"> <%= exercise.username %> </span>
<span class="track-icon"><%= track_icon(exercise.problem.track_id, 20) %></span>
Copy link

Choose a reason for hiding this comment

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

Is it possible to get some alt-text for this icon too?

Copy link

@Insti Insti Aug 22, 2016

Choose a reason for hiding this comment

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

@radar that will require changing the track_icon view helper and probably belongs in a different PR, will you create a new issue for it please?

@kytrinyx
Copy link
Member

I merged it manually in 0de777d

@kytrinyx kytrinyx closed this Aug 24, 2016
@kytrinyx
Copy link
Member

Thank you so much @bradhvr!

@Insti Insti removed the ready label Aug 25, 2016
@bradhvr bradhvr deleted the add_languange_icon_to_team_stream branch August 25, 2016 11:32
@bradhvr
Copy link
Author

bradhvr commented Aug 25, 2016

@kytrinyx, I think my CSS change may inadvertently have gotten dropped in the merge. It looks like the compiled application.css is missing my change in inbox.scss:

.track-icon {
  float: right;
}

Here's what I see in my dev environment:
image

What would be the best way to fix this?

@kytrinyx
Copy link
Member

I'll recompile locally and see if it gets added.

@kytrinyx
Copy link
Member

I think that worked. Thanks for catching that.

@bradhvr
Copy link
Author

bradhvr commented Aug 25, 2016

Great, thanks!

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

5 participants