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

update how inputs are passed to ensure all new props are forwarded as… #24

Merged

Conversation

housseindjirdeh
Copy link
Collaborator

@housseindjirdeh housseindjirdeh commented Aug 15, 2023

This PR does the following:

  • Fixes Forward remaining arguments #23. This PR updates how inputs are handled to always forward additional arguments/properties provided by the user if they don't exist somewhere in the schema (as a URL param or already specified default attribute).

    For example: A React consumer that specifies <GoogleMapsEmbed ... randomAttr="123"/> will result in <iframe ... randomAttr="123">

  • Fixes Support mapMode as a user parameter for Google Maps #25. Introduces a slugParam property to the schema that when defined by the user, either appends or replaces the slug in the src attribute URL

    For example, currently the src URL for the Google Maps Embed export is https://www.google.com/maps/embed/v1/place, but a new slugParam with a value of mode was added.

    --> If a React consumer specifies <GoogleMapsEmbed ... mode="view" />, the output becomes <iframe ... src="[123](https://www.google.com/maps/embed/v1/view">

};

const inputs = {
id: '123',

Choose a reason for hiding this comment

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

If we pass in inputs like { id: "props.id" } will that be inlined the same? We should highlight that in the test to make it clear

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean if the value is a clear string like "props.id"? It'll inline the same way:

<iframe id="props.id"></iframe>

Or are you referring to a templating logic here?

@@ -8,6 +8,7 @@
"loading": "lazy",
"src": {
"url": "https://www.google.com/maps/embed/v1/place",

Choose a reason for hiding this comment

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

Should this be "https://www.google.com/maps/embed/v1/"

So when we pass slugParam it would be appended correctly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I set up slugParam to replace the last slug so that /place is the default, but I can change the logic if that would make more sense? This way, without specifying mode will still result in this default URL

Choose a reason for hiding this comment

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

Yes, that sounds better


const result = formatData(data, inputs);
expect(result.html).toEqual(
'<iframe loading="auto" src="https://www.example.com/?id=props.id" width="150" height="100"></iframe>',

Choose a reason for hiding this comment

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

I'm not sure if this is right src="https://www.example.com/?id=props.id" since it appends id=props.id and I was thinking it would be like id={props.id}
I think I might be misunderstanding these tests and we don't have to do this. I can test the actual api once updated and revisit this later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah let me merge this in for you so you can test it out

attributes: {
loading: 'lazy',
src: {
url: 'https://www.google.com/maps/embed/v1/place',

Choose a reason for hiding this comment

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

Oh, does this replace only the last part of the url?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep!

@housseindjirdeh housseindjirdeh merged commit d460c18 into GoogleChromeLabs:main Aug 16, 2023
2 checks passed
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.

Support mapMode as a user parameter for Google Maps Forward remaining arguments
2 participants