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

Implemented Native confirm component instead of JavaScript confirm instance #5688

Conversation

PR4NJ41
Copy link
Contributor

@PR4NJ41 PR4NJ41 commented Feb 29, 2024

What this PR does

Screenshots

Before:
Before

After:
After

@PR4NJ41 PR4NJ41 changed the title Implemented Native confirm component instead of JavaScript confirm instances for assignments deletion. Implemented Native confirm component instead of JavaScript confirm instance Feb 29, 2024
@PR4NJ41
Copy link
Contributor Author

PR4NJ41 commented Mar 1, 2024

@ragesoss This PR is ready for Review. Please review it. Thank You!

@ragesoss
Copy link
Member

ragesoss commented Mar 1, 2024

Thanks! Does this feature get exercised in the test suite?

@PR4NJ41
Copy link
Contributor Author

PR4NJ41 commented Mar 1, 2024

@ragesoss Yes,a test is present in rspec. You can see the rspec here:

it 'allows instructor to remove an available article' do
pending 'This sometimes fails for unknown reasons.'
stub_info_query
stub_raw_action
Assignment.destroy_all
login_as(admin)
stub_oauth_edit
course = Course.first
wiki = Wiki.first
AssignmentManager.new(user_id: nil,
course:,
wiki:,
title: 'Education',
role: 0).create_assignment
js_visit "/courses/#{slug}/articles/available"
assigned_articles_section = page.find(:css, '#available-articles', match: :first)
expect(assigned_articles_section).to have_content 'Education'
expect(Assignment.count).to eq(1)
expect(assigned_articles_section).to have_content 'Remove'
accept_alert do
click_button 'Remove'
end
expect(assigned_articles_section).not_to have_content 'Education'
pass_pending_spec
end

@ragesoss
Copy link
Member

ragesoss commented Mar 1, 2024

You should change that spec and make sure it passes locally.

@PR4NJ41
Copy link
Contributor Author

PR4NJ41 commented Mar 1, 2024

@ragesoss I think it passes for the changes I made. But some other checks are failing (flaky)

On Master:

Before

After:

After

@PR4NJ41
Copy link
Contributor Author

PR4NJ41 commented Mar 14, 2024

@ragesoss This PR is ready for Review. Please review it. Thank You!

@ragesoss ragesoss merged commit ab15ec8 into WikiEducationFoundation:master Mar 14, 2024
1 check passed
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.

Replace all javascript confirm and alert instances with Confirm and Notification components
2 participants