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

Panel front end #638

Merged
merged 15 commits into from
Oct 8, 2020
Merged

Panel front end #638

merged 15 commits into from
Oct 8, 2020

Conversation

jseraf
Copy link
Collaborator

@jseraf jseraf commented Oct 6, 2020

Closes #629
Closes #628

@jseraf jseraf self-assigned this Oct 6, 2020
@jseraf jseraf added the panel label Oct 6, 2020
@jseraf jseraf requested a review from fanglinnw October 6, 2020 18:10
app/views/panels/edit.html.haml Outdated Show resolved Hide resolved
app/policies/panel_policy.rb Outdated Show resolved Hide resolved
@fanglinnw fanglinnw self-requested a review October 7, 2020 21:09
.cell.small-12
= f.submit 'Update Panel Information', class: 'button'

:javascript
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be separated out to its own js file and then included here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agreed, but i'm going to leave this for now. there are other files (e.g. grant form) where this needs to happen.

%h2
Review Panel
%strong
CHANGE THIS LOGIC
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this supposed to be changed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is coming up in the next PR...

end

it 'does not require end_datetime and start_dateime to be set' do
panel.start_datetime = nil
panel.end_datetime = nil
expect(panel).to be_valid
end
end

context 'meeting_link' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should potentially be separated. You can also use shared_examples to call these.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added in 6e97a80

end
end

context 'before start_datetime' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe use shared_examples here also.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added in 6e97a80

visit edit_grant_panel_path(grant)
end

scenario 'can visit the edit page' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

These could also be handled by shared examples.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added in 6e97a80

visit edit_grant_panel_path(grant)
end

scenario 'may update' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

shared examples

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added in 6e97a80

let(:grant) { create(:published_open_grant_with_users, :with_reviewer) }
let(:new_user) { create(:user) }

context 'roles' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good place for shared examples.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

since these are testing different, dynamically-created methods, i'm going to leave them as is

@jseraf
Copy link
Collaborator Author

jseraf commented Oct 8, 2020

@mattbaumann1 approval via conversation in Teams

@jseraf jseraf merged commit 9a39c57 into feature/panels Oct 8, 2020
@jseraf jseraf deleted the panel-front-end branch October 8, 2020 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants