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

Feature/events #36

Merged
merged 7 commits into from
Aug 22, 2019
Merged

Feature/events #36

merged 7 commits into from
Aug 22, 2019

Conversation

tsubik
Copy link
Contributor

@tsubik tsubik commented Aug 21, 2019

No description provided.

@tsubik tsubik added the WIP label Aug 21, 2019
@tsubik tsubik removed the WIP label Aug 21, 2019
@tsubik tsubik requested a review from kowal August 21, 2019 09:03
@tsubik tsubik marked this pull request as ready for review August 21, 2019 09:03
return unless array.respond_to?(:map)

array.map { |s| [s.humanize, s] }
array.map { |s| [s.send(transform_func), s] }
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a respond_to? check here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

has_and_belongs_to_many :legislations

accepts_nested_attributes_for :documents, allow_destroy: true
accepts_nested_attributes_for :litigation_sides, allow_destroy: true
accepts_nested_attributes_for :documents, allow_destroy: true, reject_if: :all_blank
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe:

with_options allow_destroy: true, reject_if: :all_blank do
   accepts_nested_attributes_for :documents
   accepts_nested_attributes_for :litigation_sides
   accepts_nested_attributes_for :events
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

validates :event_type, presence: true, inclusion: {in: :valid_types}

def valid_types
return Litigation::EVENT_TYPES if eventable.is_a?(Litigation)
Copy link
Contributor

Choose a reason for hiding this comment

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

here I would just put eventable.event_types and leave validation for "eventable" model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Validation must be here I guess? but I could add event_types to eventable model sure that's a good idea 👍 but that method must be implemented by all eventables.

@kowal
Copy link
Contributor

kowal commented Aug 22, 2019

There is a difference in order of tabs between show and edit pages:

Show:

Zrzut ekranu 2019-08-22 o 11 59 39

Edit:

Zrzut ekranu 2019-08-22 o 11 59 46

For me the order could be: Details - Sides - Events - (Documents) on both add/edit page.

@tsubik
Copy link
Contributor Author

tsubik commented Aug 22, 2019

All right, I will change that. Documents won't have tab (at least for now), that's only list of links

@tsubik
Copy link
Contributor Author

tsubik commented Aug 22, 2019

Ready again @kowal

Copy link
Contributor

@kowal kowal left a comment

Choose a reason for hiding this comment

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

🏅

@kowal kowal merged commit 5153ca2 into develop Aug 22, 2019
@kowal kowal deleted the feature/events branch August 22, 2019 13:40
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.

2 participants