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

deps: Upgrade to stimulus 3.0 #1890

Merged
merged 3 commits into from Jan 26, 2022
Merged

deps: Upgrade to stimulus 3.0 #1890

merged 3 commits into from Jan 26, 2022

Conversation

thi4go
Copy link
Member

@thi4go thi4go commented Jan 8, 2022

This diff updates the stimulus package to the latest version (3.0.1). It
updates data targets and uses the new API for data mapping on the
controllers and templates.

closes #1859

@thi4go thi4go changed the title [wip] deps: Upgrade to stimulus 3.0 deps: Upgrade to stimulus 3.0 Jan 10, 2022
@thi4go thi4go marked this pull request as ready for review January 10, 2022 15:44
@chappjc
Copy link
Member

chappjc commented Jan 12, 2022

I will get you a proper review soon, but one thing I noticed is the new Data Mappings https://onrails.blog/2020/12/04/updating-to-stimulus-js-2-0/

so changing

<div data-controller="meta-state" data-meta-state-key="account-id">

to

<div data-controller="meta-state" data-meta-state-key-value="account-id">

you then change this.data.get("key") -> this.keyValue

Any value to doing that? I can't tell if the old get way is deprecated or not.

@thi4go
Copy link
Member Author

thi4go commented Jan 12, 2022

Hey mate @chappjc please take your time, no rush at all to review this 👍

Upon reading more on the new data value mapping API, we should definitely update this as well, since it provides some cool additional features.

  • The previous data mapping API only works with strings, so we had to manually convert it when needed:

    height = parseInt(this.data.get('height'))

    The new API handles this type conversion automatically. It introduces a static values object to the controllers, where
    we can specify the data key and its type.

    static values = {
      url: String,
      height: Number
    }
    

    It also generates three properties for each object entry.

    this.urlValue     // getter
    this.urlValue =   // setter
    this.hasUrlValue  // exists
    

Besides the out of the box type conversions and properties, imo it adds documentation value to the controller, since we can check the types being handled on a glance, while being a cleaner syntax.

I think they will not drop support for the old API, but we should also update this to reap the improvements benefits and be up-to-date with the package. I'll get to it and commit these changes 👍

Reference: hotwired/stimulus#202

@chappjc
Copy link
Member

chappjc commented Jan 12, 2022

On a related note, I'd really love if you could follow up on a few more bootstrap 5 quirks I noticed.
If you open one tab with https://tip.dcrdata.org/ and another at https://dcrdata.decred.org/ and toggle back and forth you'll see a few minor formatting glitches.

  • Mempool section header on home page shifting left
  • Voting section header on the home page shifting up
  • A few text size deviations like "more blocks..." and "votes approved" and "more transactions..."

It may well be that we'll decide that some of these things are "better", but we're going to be upgrading the primary site for 1.7 pretty soon and we'll loose the reference and what we have on tip will become the new formatting whether it's right or not.

Awesome work on everything @thi4go !

@thi4go thi4go changed the title deps: Upgrade to stimulus 3.0 wip: deps: Upgrade to stimulus 3.0 Jan 18, 2022
@thi4go
Copy link
Member Author

thi4go commented Jan 18, 2022

My pleasure @chappjc had some trouble with my pgsql env, so took a while to test everything out. This is ready for a review mate. Will open a fresh PR for the bootstrap 5 fixes soon as well 👍

@thi4go thi4go changed the title wip: deps: Upgrade to stimulus 3.0 deps: Upgrade to stimulus 3.0 Jan 18, 2022
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

Ready for this as-is. Will take this in stages and get to the data mappings discussed in #1890 (comment) in a subsequent PR

@chappjc chappjc merged commit a358600 into decred:master Jan 26, 2022
@chappjc chappjc added this to the 6.1 milestone Nov 2, 2022
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.

upgrade to stimulus 3.0
2 participants