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

Add Address model, Factory, Addresses::Country and Addresses::Region #635

Merged
merged 10 commits into from
Sep 1, 2023

Conversation

pascallaliberte
Copy link
Member

@pascallaliberte pascallaliberte commented Jan 26, 2023

Closes #631

Joint PR:

To Do:

  • Add Address model
  • Add Addresses Factory
  • Add Addresses::Country
  • Add Addresses::Region
  • Update tangible_thing test for address field country_id dependent fields

@olbrich
Copy link
Contributor

olbrich commented Feb 16, 2023

This looks really nice. FYI, I already have an 'Address' model in my own app, seems like this might conflict with it in a few places. Any way to make it optional?

@pascallaliberte
Copy link
Member Author

This looks really nice. FYI, I already have an 'Address' model in my own app, seems like this might conflict with it in a few places. Any way to make it optional?

Once you pull from the main branch, this line will be merged into your app/models/address.rb. You can just remove it locally.

include Addresses::Base

Same with the extend line on line 2:

class Address < ApplicationRecord
extend ActiveHash::Associations::ActiveRecordExtensions
include Addresses::Base
# 🚅 add concerns above.

@jagthedrummer
Copy link
Contributor

@pascallaliberte, what's the status on this? Is this still something that you'd like to see merged?

@pascallaliberte
Copy link
Member Author

@jagthedrummer

@pascallaliberte, what's the status on this? Is this still something that you'd like to see merged?

I'll do a check next week on the joint PRs to button things up again and let you know.

@jagthedrummer jagthedrummer marked this pull request as draft August 22, 2023 19:04
@pascallaliberte pascallaliberte marked this pull request as ready for review August 29, 2023 15:14
@pascallaliberte
Copy link
Member Author

@jagthedrummer it seems it's just the Release tests that don't pass because use-core-repo is set to false. Assuming this is expected behaviour?

Otherwise, it's ready to merge.

The associated PR bullet-train-co/bullet_train-core#86 is ready to go as well: merge and release js packages.

@jagthedrummer
Copy link
Contributor

@pascallaliberte, yes, I would expect tests to fail in the Release: jobs since this PR requires the new functionality that you added in the joint PR in core. The failures help us know when we need to release a new version of core in order to accommodate changes in the starter repo.

@jagthedrummer
Copy link
Contributor

@pascallaliberte, I just pulled this down and played with it and it's super cool! Great work!

There's now a merge conflict on the joint PR over in core. But once that's resolved I think this is ready for merging.

@jagthedrummer
Copy link
Contributor

Just wanting to capture some stuff that I thought of that we'll want to do soon that relate to these PRs. Most of these are probably best handled in some follow-on PRs.

  • What's going to be someone's experience if they already have an Address model in their app with an addresses table? Where should we document our advice about how to handle this situation? (This one might be a blocker on merging and releasing this.)
  • Document the new address_field partial in the Field Partials doc
  • Add docs for the new no-config mechanism for allowing form fields to fetch themselves new options and labels
  • Document the new Stimulus controllers for dependent fields
  • Is there anything that has to be done to make this available to Super Scaffolding?
  • Document how to take advantage of this during Super Scaffolding

@pascallaliberte
Copy link
Member Author

@jagthedrummer I had as a follow-up task to address the super-scaffolding of this type of field.

The addresses table conflict is indeed super likely.

I think we'll keep the migration in and, as everything in the starter repo, lean on people picking and choosing what they keep from upgrades from upstream, as recommended in the upgrades doc.

For docs, I can take it on in a separate PR. The new pattern for self-updating fields is indeed a new approach that deserves a bit of spotlight.

But maybe a bit more should be bundled in with this feature before release. Initially it was "let's just ship this", but maybe we're at a point where wrapping a feature like this with docs is required. Up to you.

@jagthedrummer
Copy link
Contributor

@pascallaliberte, I'm totally fine with merging this as is to avoid more merge conflicts, and then following up with the docs. Sound good to you?

@jagthedrummer jagthedrummer merged commit 29768d5 into main Sep 1, 2023
@jagthedrummer jagthedrummer deleted the add-addresses branch September 1, 2023 14:15
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.

Add an address multi-field partial
4 participants