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 programming language icons #2138

Merged
merged 15 commits into from
Aug 10, 2020
Merged

Conversation

winniederidder
Copy link
Contributor

This pull request uses different icons for activities depending on their Programming Language.

Sample of a current table:
new_icons

Current issues:
Example testing exercises are always linked to Python, so e.g. Echo JS shows a Python logo.
Blank, a.k.a. Programming Language = text is also not available yet to test visually.

Exercises are created via a git repo, so I require explanation on how to work with that.
Closes #1975 .

@winniederidder winniederidder requested review from bmesuere and rien August 3, 2020 14:50
Copy link
Member

@rien rien left a comment

Choose a reason for hiding this comment

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

Can you update your screenshot with an exercise for each language?
You can change an exercise's programming language trough the console.

@winniederidder
Copy link
Contributor Author

Picture with icons from different languages.
new_icons

@winniederidder winniederidder requested a review from rien August 4, 2020 11:17
app/views/activities/_activities_table.html.erb Outdated Show resolved Hide resolved
app/views/activities/_series_activities_table.html.erb Outdated Show resolved Hide resolved
app/views/evaluations/_exercises_progress_table.html.erb Outdated Show resolved Hide resolved
app/views/evaluations/_exercises_table.html.erb Outdated Show resolved Hide resolved
@pdawyndt
Copy link
Contributor

pdawyndt commented Aug 4, 2020

If we also attach a programming language to a submission (inherited from the exercise for now), we could also add the programming language icon to all tables showing a list of submissions. If feasible, this could be turned into a new issue.

@winniederidder winniederidder requested a review from rien August 4, 2020 11:40
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

Also add the programming language to the title tag (and thus tooltip) of the icon ("exercise" -> "python exercise").

@rien don't we want to handle the "no icon" case in the model so we don't have to add that exception each time we use it?

@rien
Copy link
Member

rien commented Aug 4, 2020

@rien don't we want to handle the "no icon" case in the model so we don't have to add that exception each time we use it?

I would maybe put it in a helper, because otherwise we're mixing application logic with its views.

@pdawyndt
Copy link
Contributor

pdawyndt commented Aug 4, 2020

Also useful to show the icons in the overview of learning activities for a series in the right margin (navigation element) and also on the exercise page itself (after the title in the header?).

@bmesuere
Copy link
Member

bmesuere commented Aug 4, 2020

@rien don't we want to handle the "no icon" case in the model so we don't have to add that exception each time we use it?

I would maybe put it in a helper, because otherwise we're mixing application logic with its views.

Yes, but this is logic, right? It seems logical to overwrite the icon attribute in the model and return the string from the datababase or "file-document-edit-outline". That way there's one place where de default icon is set and you can trust the icon call to return something useful.

@rien
Copy link
Member

rien commented Aug 4, 2020

Yes, but this is logic, right? It seems logical to overwrite the icon attribute in the model and return the string from the datababase or "file-document-edit-outline". That way there's one place where de default icon is set and you can trust the icon call to return something useful.

Hmm yes, you're right. I think putting the icon in the database was mixing logic and views with each other to begin with. But adding a constant mapping in the ProgrammingLanguage class is also not ideal.

I would indeed write something in the model like

def icon
  self[:icon] || "file-document-edit-outline"
end

@bmesuere
Copy link
Member

bmesuere commented Aug 4, 2020

@winniederidder can you modify the code as proposed by @rien and add the title?

@winniederidder
Copy link
Contributor Author

Updated tooltip visual:
new_icons

@winniederidder winniederidder requested a review from bmesuere August 5, 2020 06:47
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

The tests still seem to fail. Probably a missing &. somewhere.

@winniederidder
Copy link
Contributor Author

The tests still seem to fail. Probably a missing &. somewhere.

Was working on that while also updating the i18n
Visuals now:
new_icons

@winniederidder winniederidder requested a review from bmesuere August 5, 2020 08:23
@winniederidder winniederidder requested a review from bmesuere August 6, 2020 07:19
Copy link
Member

@rien rien left a comment

Choose a reason for hiding this comment

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

👌

Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

Looks good, but I don't think the file test/remotes/exercises/public/CodersApprentice.png should be in this PR. Ping me if removed so I can merge.

@bmesuere bmesuere merged commit d82e43f into develop Aug 10, 2020
@bmesuere bmesuere deleted the feature/programming-language-icons branch August 10, 2020 06:19
@bmesuere bmesuere changed the title Feature/programming language icons Add programming language icons Aug 12, 2020
@bmesuere bmesuere added the feature New feature or request label Aug 12, 2020
@bmesuere bmesuere added this to the 3.8 milestone Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use language-specific icons for programming exercises
5 participants