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

Addressable getFrontendFields issues #61

Merged
merged 1 commit into from
Jun 1, 2020
Merged

Addressable getFrontendFields issues #61

merged 1 commit into from
Jun 1, 2020

Conversation

phyzical
Copy link

I have not updated any tests, i cant seem to successfully run them.

if you know of a quick guide to running the tests ill give it a shot to make sure nothing is broken

#60

I understand this was trying to avoid overriding existing fields but i never receive the fields this solution's would simply add or override.

unless you guys can think of a better way to handle this?

@phyzical phyzical changed the title Removed a check as merge simply overrides keys Addressable getFrontendFields issues Sep 21, 2018
@stephenmcm
Copy link
Contributor

Hi @phyzical
Someone will need to spin the code up to have a look, from what I can see it is a bug and should be returning the addressable fields but I'm not super familiar with this part of the module.

If no one has looked at this in a week or so give us a nudge and I'll work something out.

@phyzical
Copy link
Author

sorry maybe i should clarify incase someone misinterprets ,

i get all the fields how it is, but they all come in as TextFields due to the explanation in #60 rather than using the getAddressFields func with dropdowns ect.

@phyzical
Copy link
Author

friendly bump :)

@phyzical
Copy link
Author

one more bump <3

@nglasl
Copy link
Contributor

nglasl commented Jan 7, 2019

Thanks for bumping this, @stephenmcm could you please have a look at this one? Just for future reference @phyzical, please ping someone directly as it's easy to miss notifications when a pull request or issue isn't marked as watching :)

Also, are you okay with me closing the issue while this pull request remains open? I'm keen to keep all the conversations in a single thread so it's easy to follow.

@nglasl
Copy link
Contributor

nglasl commented Mar 28, 2019

@stephenmcm another bump on this one.

@nglasl nglasl requested review from JoshuaCarter and removed request for stephenmcm October 1, 2019 05:41
@nglasl nglasl assigned JoshuaCarter and unassigned stephenmcm Oct 1, 2019
@nglasl
Copy link
Contributor

nglasl commented Oct 1, 2019

Apologies for this one taking so long, I've updated the assignment here in an effort to get things moving.

Copy link
Contributor

@nglasl nglasl left a comment

Choose a reason for hiding this comment

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

Considering how long it has taken to get this simple change moving, I'm going to approve and merge this. The solution seems fine to me on the surface based on your comments.

@nglasl nglasl merged commit f65c4c2 into symbiote:master Jun 1, 2020
@phyzical phyzical deleted the ISSUE-60 branch June 20, 2020 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants