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.

Sign up for GitHub

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

Jump to bottom

Feature: Hide live preview input and button #3968

Merged
merged 1 commit into from
Jul 21, 2023
Merged

Feature: Hide live preview input and button #3968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion app/components/project_submissions/item_component.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@

<div class="flex flex-row md:items-center">
<%= link_to 'View code', project_submission.repo_url, target: '_blank', rel: 'noreferrer', class: 'button button--gray font-semibold mr-4', data: { test_id: 'view-code-btn' } %>
<%= link_to 'Live preview', project_submission.live_preview_url, target: '_blank', rel: 'noreferrer', class: 'button button--gray font-semibold mr-4', data: { test_id: 'live-preview-btn' } %>

<% if project_submission.lesson.has_live_preview? %>
Copy link
Member

Choose a reason for hiding this comment

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

Mega nit: Demeter

A delegated previewable? or similar method might be a nice minor addition to the project submission model? I was going to suggest doing it in the component, but we're calling the same logic from the form too

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice! previewable is a much better name. What do you think about us renaming the db column to that? we'd get those nice predicate methods for free.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah mint even better, love that ❤️

Copy link
Member Author

Choose a reason for hiding this comment

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

I created a follow up issue to rename it #4003

Best to do it after the react version gets the 🪓

<%= link_to 'Live preview', project_submission.live_preview_url, target: '_blank', rel: 'noreferrer', class: 'button button--gray font-semibold mr-4', data: { test_id: 'live-preview-btn' } %>
<% end %>

<div class="flex-none absolute top-7 right-0 md:relative md:top-auto md:right-auto" data-controller="visibility" data-action="visibility:click:outside->visibility#off" data-visibility-visible-value="false">
<button type="button" data-action="click->visibility#toggle" data-test-id="submission-action-menu-btn" class="-m-2.5 block p-2.5 text-gray-500 hover:text-gray-900 dark:text-gray-300 dark:hover:text-gray-100" id="options-menu-0-button" aria-expanded="false" aria-haspopup="true">
Expand Down
14 changes: 8 additions & 6 deletions app/views/lessons/v2_project_submissions/_form.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@
<% end %>
</div>

<div>
<%= form.label :live_preview_url, 'Live preview (optional)' %>
<%= form.text_field :live_preview_url, leading_icon: true, placeholder: 'http://example.com', class: 'text-sm', data: { test_id: 'live-preview-url-field' } do %>
<%= inline_svg_tag 'icons/link.svg', class: 'h-5 w-5 text-gray-400', aria: true, title: 'Project live preview', desc: 'Link icon' %>
<% end %>
</div>
<% if project_submission.lesson.has_live_preview? %>
<div>
<%= form.label :live_preview_url, 'Live preview (optional)' %>
<%= form.text_field :live_preview_url, leading_icon: true, placeholder: 'http://example.com', class: 'text-sm', data: { test_id: 'live-preview-url-field' } do %>
<%= inline_svg_tag 'icons/link.svg', class: 'h-5 w-5 text-gray-400', aria: true, title: 'Project live preview', desc: 'Link icon' %>
<% end %>
</div>
<% end %>

<fieldset class="mt-2">
<legend class="text-sm font-medium leading-6 text-gray-900 dark:text-white">Privacy</legend>
Expand Down
3 changes: 2 additions & 1 deletion spec/support/pages/project_submissions/form.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ class Form

option :repo_url, default: -> { 'https://github.com/myname/my-project' }
option :live_preview_url, default: -> { 'https://myprojectlivepreview.com' }
option :has_live_preview, default: -> { true }

def self.fill_in_and_submit(**args)
new(**args)
Expand All @@ -22,7 +23,7 @@ def open

def fill_in
find(:test_id, 'repo-url-field').fill_in(with: @repo_url)
find(:test_id, 'live-preview-url-field').fill_in(with: @live_preview_url)
find(:test_id, 'live-preview-url-field').fill_in(with: @live_preview_url) if @has_live_preview
self
end

Expand Down
28 changes: 26 additions & 2 deletions spec/system/v2_lesson_project_submissions/add_submission_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
require 'rails_helper'

RSpec.describe 'Add a Project Submission' do
let(:lesson) { create(:lesson, :project) }

before do
Flipper.enable(:v2_project_submissions)
end
Expand All @@ -12,6 +10,7 @@
end

context 'when a user is signed in' do
let(:lesson) { create(:lesson, :project) }
let(:user) { create(:user) }
let(:another_user) { create(:user) }

Expand Down Expand Up @@ -55,8 +54,33 @@
end
end

context 'when lesson does not allow previews' do
let(:lesson) { create(:lesson, :project, has_live_preview: false) }
let(:user) { create(:user) }

before do
sign_in(user)
visit lesson_path(lesson)
end

it 'adds the submission without a preview' do
Pages::ProjectSubmissions::Form
.new(has_live_preview: false)
.open
.fill_in
.submit

within(:test_id, 'submissions-list') do
expect(page).to have_content(user.username)
expect(page).to have_link('View code')
expect(page).not_to have_link('Live preview')
Comment on lines +66 to +76
Copy link
Member

Choose a reason for hiding this comment

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

I haven't done thing kind of thing much before, but it intrigues me. Do you find it generally helpful? Is there a risk of drifting away from reality, since there's such a test specific interface? Or is it more about convenience in this case?

Curious to hear your thoughts in general on the goals / benefits / downsides of an approach like this

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry @ChargrilledChook, I'm probably being very dense lol. But is this about the page object or the line you commented on expect(page).not_to have_link('Live preview')?

Copy link
Member

Choose a reason for hiding this comment

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

No worries and my bad, re-reading my comment it is a bit cryptic / stream of consciousness 😅

You're spot on though, I'm referring to the page object Pages::ProjectSubmissions::Form

Copy link
Member Author

@KevinMulhern KevinMulhern Jul 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No worries @ChargrilledChook , happy to explain the approach

I've been using them for a few years at work / on side projects. Heavily inspired by Thoughtbot / Evil Martians and hating having to spend a lot of time figuring out what large specs were actually doing lol.

Two biggest benefits for me:

  • Readability - As time goes on and they grow, Capyabara tests tend get harder to read. Mostly because they're imperative with lots of details on how something is done, think of all the click_button, click_link and fill_in methods calls. Page objects allow for far more expressive and declarative tests - "what will be done" kind of deal. A bunch of related steps can be encapsulated and called with one easy to grok method call - like form.fill_in_and_submit.
  • Reusability - Once you have a page object, it makes all future related tests easier and quicker to write. Theres a more tangible benefit with them on larger systems where there will be a few core forms/pages which many different system tests need to interact with. For us, it's been great for the project submissions where we need different tests for editing, flagging, voting etc.

Is there a risk of drifting away from reality, since there's such a test specific interface? Or is it more about convenience in this case?

I haven't had too many issues with things drifting. But definitely see how they could. I think the trick is not to overuse them. If everything is a page object, you'd be jumping from file to file and absolutely hating the test suite.
Forms are always great candidates for page objects, they need the same steps, potentially in many specs and their interface is always pretty standard, they all need fill_in and submit methods.

These articles were a big help when I first started using them:
https://thoughtbot.com/blog/better-acceptance-tests-with-page-objects
https://evilmartians.com/chronicles/system-of-a-test-2-robust-rails-browser-testing-with-siteprism

end
end
end

context 'when a user is not signed in' do
it 'they cannot add a project submission' do
lesson = create(:lesson, :project)
visit lesson_path(lesson)

expect(page).not_to have_button('Add Solution')
Expand Down