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 type icon to reading activities and exercises #3178

Merged
merged 7 commits into from
Oct 25, 2021
Merged

Add type icon to reading activities and exercises #3178

merged 7 commits into from
Oct 25, 2021

Conversation

BTWS2
Copy link
Contributor

@BTWS2 BTWS2 commented Oct 24, 2021

This pull request adds the type icon to reading activities and exercises.

image

image

image

Doesn't account for multiple programming languages for one exercise (one-to-many relationship) because this isn't supported yet.

Related to #1124.

@BTWS2 BTWS2 requested a review from a team as a code owner October 24, 2021 15:05
@BTWS2 BTWS2 requested review from bmesuere and niknetniko and removed request for a team October 24, 2021 15:05
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 design looks good, but I would extract the code to a helper function (and replace existing occurences of similar code. The helper function will probably also take an optional size argument (e.g., 18 by default).

@BTWS2
Copy link
Contributor Author

BTWS2 commented Oct 24, 2021

I'm new to Ruby and tried these two helpers without succes:

In activity_helper.rb

def show_type_icon(activity, size)
  if activity.exercise?
    "<i class='<%= 'mdi mdi-#{activity.programming_language&.icon} mdi-#{size}' %>' title='<%= '#{t 'activities.index.type.exercise_language', language: activity.programming_language&.name&.titleize}' %>'></i>%>"
  elsif activity.content_page?
    "<i class='mdi mdi-book-open-variant mdi-#{size}' title='<%= t 'activities.index.type.content' %>'></i>"
  end
end

or

  def show_type_icon(activity, size)
    if activity.exercise?
      content_tag(:i, '', class:"mdi mdi-#{activity.programming_language&.icon} mdi-#{size}", title: "#{t 'activities.index.type.exercise_language', language: activity.programming_language&.name&.titleize}")
    elsif activity.content_page?
      content_tag(:i, '', class:"mdi mdi-book-open-variant mdi-#{size}", title: "#{t 'activities.index.type.content'}")
    end
  end

and this in show.html.erb
<% show_type_icon(@activity, 24) %>

What am I missing?

@bmesuere
Copy link
Member

I think you missed an = in the show file, so <%=, to output the return value of the function to the page.

@chvp chvp added the feature New feature or request label Oct 25, 2021
@@ -99,6 +99,14 @@ def compare_solutions(a, b)
end
end

def show_type_icon(activity, size = 18)
Copy link
Member

Choose a reason for hiding this comment

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

The name of this function isn't very Rails-y. The show is not something that we usually add. I'm also not sure about the type name. It doesn't really cover the programming language being returned in case of an exercise. I can't immediately think of a better name though.

Copy link
Member

Choose a reason for hiding this comment

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

activity_icon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

activity_icon is good for me.

@bmesuere bmesuere added deploy mestra Request a deployment on mestra and removed deploy mestra Request a deployment on mestra labels Oct 25, 2021
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.

This looks good. Because this is an external repo, the tests aren't run (not sure why) and I can't seem to do a local checkout to run them locally.

@chvp can you run the tests before merging?

@bmesuere bmesuere requested a review from chvp October 25, 2021 09:11
@thepieterdc
Copy link
Member

This looks good. Because this is an external repo, the tests aren't run (not sure why)

Because otherwise everyone can make a fork of this repo, change one of the GitHub Actions workflows to send your naos deploy key to their own server and store it.

@chvp
Copy link
Member

chvp commented Oct 25, 2021

This looks good. Because this is an external repo, the tests aren't run (not sure why)

Because otherwise everyone can make a fork of this repo, change one of the GitHub Actions workflows to send your naos deploy key to their own server and store it.

Secrets are never given for pull requests from other repositories. The actions are on push and can definitely be run on pull request too.

Copy link
Member

@niknetniko niknetniko left a comment

Choose a reason for hiding this comment

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

Rubocop detects two style issues:

Inspecting 308 files
....................................C...............................................................................................................................................................................................................................................................................

Offenses:

app/helpers/activity_helper.rb:104:103: C: [Correctable] Style/RedundantInterpolation: Prefer to_s over string interpolation.
      content_tag(:i, '', class: "mdi mdi-#{activity.programming_language&.icon} mdi-#{size}", title: "#{t 'activities.index.type.exercise_language', language: activity.programming_language&.name&.titleize}")
                                                                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/helpers/activity_helper.rb:106:82: C: [Correctable] Style/RedundantInterpolation: Prefer to_s over string interpolation.
      content_tag(:i, '', class: "mdi mdi-book-open-variant mdi-#{size}", title: "#{t 'activities.index.type.content'}")
                                                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

308 files inspected, 2 offenses detected, 2 offenses auto-correctable

Normally you can run bundle exec rubocop -a to fix the issues automatically.

@chvp
Copy link
Member

chvp commented Oct 25, 2021

Note that rubocop's suggestion isn't even necessary. We know that t will return a string.

@chvp
Copy link
Member

chvp commented Oct 25, 2021

Tests succeed.

@chvp chvp enabled auto-merge October 25, 2021 10:05
@chvp chvp merged commit 44ecfa4 into dodona-edu:develop Oct 25, 2021
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.

5 participants