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/redmine4.0 #112

Merged
merged 16 commits into from Mar 14, 2019
Merged

Feature/redmine4.0 #112

merged 16 commits into from Mar 14, 2019

Conversation

dmartingit
Copy link
Contributor

No description provided.

@dmartingit
Copy link
Contributor Author

dmartingit commented Mar 13, 2019

After testing everything manually, it turned out that some things didn't work as expected.

@dmartingit
Copy link
Contributor Author

dmartingit commented Mar 13, 2019

An exception was raised in our form fields generation because false values passed to include_blank in a select would be ignored if the field was required: rails/rails#20124

Fixed it in #112

Copy link
Member

@arBmind arBmind left a comment

Choose a reason for hiding this comment

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

Good work! This PR contains a lot of white-space changes - it's best practice to keep those to a minimum, to allow better tracking of the real changes.

@@ -2,4 +2,4 @@
.label
= form.label :activity_id
.input
= form.collection_select :activity_id, TimeEntryActivity.applicable(entry.project), :id, :name, {include_blank: !local_assigns[:required]}, disabled: local_assigns[:disabled], required: local_assigns[:required]
= form.collection_select :activity_id, TimeEntryActivity.applicable(entry.project), :id, :name, { include_blank: local_assigns[:required] }, disabled: local_assigns[:disabled], required: local_assigns[:required]
Copy link
Member

Choose a reason for hiding this comment

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

This seems odd. Removing the ! changed the logic?

Copy link
Member

Choose a reason for hiding this comment

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

Does this still work with the old Redmine / Rails versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have the following two cases:

  1. Select tag is not required -> include a blank field
  2. Select tag is required -> no blank field

What has changed is that we can no longer have a required select tag without a blank option.
The reason is that the HTML5 standard requires that behavior and rails 5 adopted it.
This leads to a changing behavior in the 2. case and the closest behavior we can create is imo to always include a blank field in a select tag.

Copy link
Member

@arBmind arBmind left a comment

Choose a reason for hiding this comment

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

looks good to me.

@dmartingit dmartingit merged commit d2bfba9 into develop Mar 14, 2019
@dmartingit dmartingit deleted the feature/redmine4.0 branch March 14, 2019 13:36
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.

None yet

2 participants