Remove status_tag type parameter #4989

javierjulio · 2017-06-03T23:17:32Z

This is a change that was requested by @timoschilling in #3677 (comment) (look at item 2) and was pulled from #3862 there since we want to incorporate it earlier and minimize the amount of changes needed for review. Also we did want to remove the additional classes as users should be able to add those on their own rather than defining ones and styles they might not want.

Before providing any options, a second argument called type was required. So you would write something like the following:

status_tag('active', :ok, class: 'abc', label: 'on')
status_tag('active', nil, class: 'abc', label: 'on')

This didn't make much sense as type was just appending to class but its just easier for users to provide their own classes for styling while ActiveAdmin just focuses on providing the defaults it handles which are true/false.

Now with type removed, the usage becomes:

status_tag('active', class: 'abc', label: 'on')

varyonic

Needs a CHANGELOG Breaking Changes description. If my code is full of status_tag(status, :ok) what does that become now? Also update 12-arbre-components.md

javierjulio · 2017-06-05T16:42:31Z

@varyonic sounds good. Just pushed updates for that. Let me know how they look. Thanks!

javierjulio · 2017-06-05T17:59:30Z

@deivid-rodriguez if you have the time would appreciate your input here too. Thanks!

deivid-rodriguez

I'm in favor of this change in general. I'm unsure whether we should provide a deprecation path and keep the old mode working in the next releases with a deprecation message...

deivid-rodriguez · 2017-06-05T18:15:55Z

lib/active_admin/views/components/status_tag.rb

@@ -60,7 +53,7 @@ def convert_to_boolean_status(status)
        case status
        when true, 'true'
          'Yes'
-        when false, 'false', nil
+        when *FALSE_VALUES


Why is the "false values" list incremented? And why is the "true values" list not incremented accordingly?

@deivid-rodriguez I don't know. I tried to see if I made the change on my own or if there was a comment in #3862 by someone to suggest otherwise but doesn't seem to be. I must have done that on my own, but don't remember. I can reset it back to what it was. Should I do that?

Yes, let's leave it as it was if we can't remember the reason for the change.

@deivid-rodriguez code updated.

javierjulio · 2017-06-05T19:40:25Z

Let me know what you end up preferring here. I realize now that the original code worked with how I was using it because of the splat. I could either revert it back to that or do something like:

def build(status, type = nil, options = {})
  if !type.is_a?(Hash)
    Deprecation.warn "the `type` parameter is deprecated! Provide as a class: `status_tag(status, class: 'abc')`."
  end

  options = type if type.is_a?(Hash)
  label = options.delete(:label)
  classes = options.delete(:class)
  status = convert_to_boolean_status(status)

  # ...
  
  add_class(status_to_class(status)) if status
  add_class(type.to_s) if type && !type.is_a?(Hash)
  add_class(classes) if classes
end

And from there we would have to update our tests as they now fail with the deprecation warning. If we go this route, do you just want the existing tests in place but just stub out the deprecation calls?

varyonic · 2017-06-05T19:42:10Z

CHANGELOG.md

+    ```ruby
+    status_tag(status, :ok, class: 'completed', label: 'on')
+    # now changes to:
+    status_tag(status, class: 'completed ok', label: 'on')


But the provided .completed and .ok classes have been removed also, no?

They have. But this just demonstrates the expected output. I could add a note that previous classes were also removed too.

Yes, as someone who has to make this change I now have to go define these styles as part of the upgrade.

@varyonic changelog updated.

javierjulio · 2017-06-05T19:46:09Z

The build failure has to do with phantomjs. Although I see in the response log outputted about a GitHub request and it had some outage in the past hour so that's most likely the culprit.

deivid-rodriguez · 2017-06-05T20:16:46Z

@javierjulio True, the type parameter was actually optional... I'm not really sure now. With "reverting back" you mean ditching this whole PR? What are the reasons for removing the type parameter anyways?

In any case, if we're going to make the breaking change, let's add the deprecation since it seems easy enough as you just proved and can help users get ready for the new way.

Regarding tests, let's take care of that once we make a decision.

javierjulio · 2017-06-05T20:36:21Z

@deivid-rodriguez by reverting meaning going back to using splat. I removed the splat based on comments made in #3862. The type param was removed because that just set the CSS class when users can set that themselves and makes the method signature simpler. The param was requested to be removed by @timoschilling, the comment is linked in the PR description. I'll make the necessary updates and stub the deprecation calls in the tests.

deivid-rodriguez · 2017-06-05T20:48:45Z

@javierjulio Sorry I didn't properly read through the link. Actually that comment also answers the previous question about boolean list (bullet point 3). Let's leave that for now anyways.

Your plan sounds good to me, thanks for your work!

javierjulio · 2017-06-05T21:18:52Z

@deivid-rodriguez fixing the unit tests isn't bad but since I'm not familiar working with cucumber much, I'm not sure the best way to stub out the deprecation warnings as that is now causing many cucumber failures. Any ideas?

deivid-rodriguez · 2017-06-05T22:18:07Z

lib/active_admin/views/components/status_tag.rb

@@ -50,7 +47,7 @@ def build(*args)
        super(content, options)

        add_class(status_to_class(status)) if status
-        add_class(type.to_s) if type
+        add_class(type.to_s) if type && !type.is_a?(Hash)


Regarding the implementation, after a second read I think it's better to keep the splat. I think we can actually leave this method exactly as it was, but just changing this:

- add_class(type.to_s) if type + if type + Deprecation.warn "the `type` parameter is deprecated! Provide as a class: `status_tag(status, class: 'abc')`." + add_class(type.to_s) + end

Yes it will be better and less work. Cucumber tests are passing with it going back to being a splat. 👊🏻

deivid-rodriguez · 2017-06-05T22:23:02Z

Don't know a lot about cucumber either, but will try to help!

javierjulio · 2017-06-06T00:07:38Z

@deivid-rodriguez you won't need too! As I thought when you suggested going back to the splat version, the cucumber tests wouldn't be an issue anymore. And they aren't. 😏 Tests are all green locally. I think with this we've addressed everything. Let me know what you and @varyonic think. Its coming together.

varyonic

Nice.

deivid-rodriguez · 2017-06-06T15:08:17Z

CHANGELOG.md

+      &.warn, &.warning, &.orange { background: #e29b20; }
+      &.error, &.errored, &.red { background: #d45f53; }
+    }
+    ```


This is no longer a breaking change (not yet). I guess we should move it to a "deprecations" section or something like that.

I'll move it to a Deprecations section instead.

Hmm, removal of the CSS classes still makes it a breaking change IMHO.

deivid-rodriguez · 2017-06-06T15:14:25Z

CHANGELOG.md

+    ```ruby
+    status_tag(status, :ok, class: 'completed', label: 'on')
+    # now changes to:
+    status_tag(status, class: 'completed', label: 'on')


Does the new code not need the "ok" class as well?

Yes it can be added to demonstrate that's what it was originally doing and to maintain the same behavior it would have to be added if they were still using that CSS class.

javierjulio · 2017-06-06T15:38:35Z

@deivid-rodriguez renamed section as Deprecations and rewrote that that type param is now deprecated.

deivid-rodriguez · 2017-06-06T15:51:14Z

app/assets/stylesheets/active_admin/components/_status_tags.scss

-  &.ok, &.published, &.complete, &.completed, &.green { background: #8daa92; }
-  &.warn, &.warning, &.orange { background: #e29b20; }
-  &.error, &.errored, &.red { background: #d45f53; }
-


I don't think it makes sense to keep compatibility with a deprecation, but remove the styles, since people's designs would break anyways. I think we should add TODO note here to "schedule removal" when we actually remove the feature. Would that get in the middle somehow with the rest of the css overhaul?

Yes that's fine. We can deprecate the CSS. I'll have to be careful when rebasing on the css overhaul that it doesn't get lost. I will still keep the CSS in the changelog though and note that its deprecated so if users want it they should copy it themselves.

deivid-rodriguez · 2017-06-06T15:52:50Z

@javierjulio I added yet another comment. Sorry for the extra pickyness here, but I think the better work we do now, the less work we'll have later...

Second thoughts about removing the classes if this is just a deprecation.

deivid-rodriguez

Left a minor last comment about the deprecation message but looks good to me now!

deivid-rodriguez · 2017-06-06T16:21:15Z

lib/active_admin/views/components/status_tag.rb

-        add_class(type.to_s) if type
+
+        if type
+          Deprecation.warn "the `type` parameter is deprecated! Provide as a class: `status_tag(status, class: 'abc')`."


I think we can still improve the message a bit. What about this:

The type parameter has been deprecated. Provide the "#{type}" type as a class option instead. For example, status_tag(status, :ok, class: "abc") would change to status_tag(status, class: "ok abc"). Also note that the #{type} CSS class will be removed too, so you'll have to provide the CSS rules yourself. See https://github.com/activeadmin/activeadmin/blob/master/CHANGELOG.md#110- for more information.

Can't be that detail so it looks great to me!

I just tweaked it a little bit:

The `type` parameter has been deprecated. Provide the "#{type}" type as a class instead. For example, `status_tag(status, :ok, class: "abc")` would change to `status_tag(status, class: "ok abc")`. Also note that the "#{type}" CSS rule will be removed too, so you'll have to provide the styles yourself. See https://github.com/activeadmin/activeadmin/blob/master/CHANGELOG.md#110- for more information.

javierjulio · 2017-06-06T17:14:37Z

@deivid-rodriguez updated the message. Also made sure the full status_tag example in the message evaluated the type param so it output the value specific to that users' context.

deivid-rodriguez · 2017-06-06T17:24:36Z

It looks great, thanks!

javierjulio · 2017-06-06T17:51:24Z

@varyonic not sure why you dismissed your review but in case you wanted to review again, I addressed the latest issues regarding the CSS and deprecation message. Let me know what you think.

deivid-rodriguez · 2017-06-06T17:56:12Z

It was when we noticed that removing the CSS was actually a breaking change. He'll probably reapprove when he sees this! 😉

varyonic

OK but squash commits.

We can provide existing functionality here pretty easily so we’ll deprecate the type param and update our unit tests to suppress the warning. The tests for the specific type param check the warning message to make sure its formatted right. Added changelog for `status_tag` breaking change. Updated examples in documentation. Providing a more detailed deprecation message. When given a type of “ok” the output from running the code in a console is: ``` DEPRECATION WARNING: Active Admin: The `type` parameter has been deprecated. Provide the "ok" type as a class instead. For example, `status_tag(status, :ok, class: "abc")` would change to `status_tag(status, class: "ok abc")`. Also note that the "ok" CSS rule will be removed too, so you'll have to provide the styles yourself. See https://github.com/activeadmin/activeadmin/blob/master/CHANGELOG.md#110- for more information. ```

javierjulio · 2017-06-06T19:48:18Z

@varyonic I wasn't sure whether to squash related commits or just down to one but did the latter. Kept the meaningful messages intact that mention deprecation, not removing. Thanks. Let me know how it is. Thanks for the help @deivid-rodriguez!

deivid-rodriguez · 2017-06-06T20:12:05Z

Yeah, both were fine! Thanks, @javierjulio!!

seanlinsley · 2018-01-09T17:24:40Z

If we're going to remove the default status_tag colors, I think it'd make sense to move them into the documentation as a suggested starting point.

Developers aren't the best designers, so having a color scheme to start from can help them build a better information architecture.

varyonic suggested changes Jun 4, 2017

View reviewed changes

deivid-rodriguez reviewed Jun 5, 2017

View reviewed changes

varyonic reviewed Jun 5, 2017

View reviewed changes

deivid-rodriguez reviewed Jun 5, 2017

View reviewed changes

varyonic previously approved these changes Jun 6, 2017

View reviewed changes

deivid-rodriguez reviewed Jun 6, 2017

View reviewed changes

deivid-rodriguez approved these changes Jun 6, 2017

View reviewed changes

javierjulio force-pushed the remove-status-tag-type branch from 5195209 to c03b8f7 Compare June 6, 2017 17:13

javierjulio force-pushed the remove-status-tag-type branch from 07cd002 to 22df879 Compare June 6, 2017 17:49

varyonic reviewed Jun 6, 2017

View reviewed changes

javierjulio force-pushed the remove-status-tag-type branch 2 times, most recently from 45dfe8b to 511b175 Compare June 6, 2017 19:42

javierjulio force-pushed the remove-status-tag-type branch from 511b175 to 484a051 Compare June 6, 2017 19:45

varyonic merged commit 716a1b3 into activeadmin:master Jun 7, 2017

Ivanov-Anton mentioned this pull request Apr 15, 2020

upgrade activeadmin yeti-switch/yeti-web#674

Merged

Remove status_tag type parameter #4989

Remove status_tag type parameter #4989

Conversation

javierjulio commented Jun 3, 2017 • edited Loading

varyonic left a comment • edited Loading

Choose a reason for hiding this comment

javierjulio commented Jun 5, 2017

javierjulio commented Jun 5, 2017

deivid-rodriguez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javierjulio commented Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javierjulio commented Jun 5, 2017

deivid-rodriguez commented Jun 5, 2017

javierjulio commented Jun 5, 2017

deivid-rodriguez commented Jun 5, 2017

javierjulio commented Jun 5, 2017

deivid-rodriguez Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deivid-rodriguez commented Jun 5, 2017 • edited Loading

javierjulio commented Jun 6, 2017

varyonic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javierjulio commented Jun 6, 2017

deivid-rodriguez Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deivid-rodriguez commented Jun 6, 2017

deivid-rodriguez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javierjulio commented Jun 6, 2017 • edited Loading

deivid-rodriguez commented Jun 6, 2017

javierjulio commented Jun 6, 2017

deivid-rodriguez commented Jun 6, 2017

varyonic left a comment • edited Loading

Choose a reason for hiding this comment

javierjulio commented Jun 6, 2017

deivid-rodriguez commented Jun 6, 2017

seanlinsley commented Jan 9, 2018

javierjulio commented Jun 3, 2017 •

edited

Loading

varyonic left a comment •

edited

Loading

javierjulio commented Jun 5, 2017 •

edited

Loading

deivid-rodriguez Jun 5, 2017 •

edited

Loading

deivid-rodriguez commented Jun 5, 2017 •

edited

Loading

deivid-rodriguez Jun 6, 2017 •

edited

Loading

javierjulio commented Jun 6, 2017 •

edited

Loading

varyonic left a comment •

edited

Loading