Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MWPW-153962: Introduce maslibs query parameter #2544
MWPW-153962: Introduce maslibs query parameter #2544
Changes from 13 commits
dbf15b0
6e5d64f
ee41ef3
06733c0
a2f0a03
b7656eb
5230f70
8e265fb
0396a8a
58d71c0
f6da603
9f12583
8d6dc0b
8492f90
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would always be empty on the first pass, right? I think our pattern here is to expose a
let baseUrl
outside the method and then update the value. https://github.com/adobecom/milo/pull/2544/files#diff-01476d99c8ba4d00b7e767d0afbc424902d0d40e75c4ec4272a966edd0d4124dR149 is a bit confusingThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used this pattern in the other functions in merch block. Thus, we can cache the calculated value, and erase it when needed (mostly in tests). Using a module scope variable makes testing harder. You need to provide a utility method to reset the module scope variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But still, on the first pass the baseUrl value would be empty and I don't see a reason why you would need to call the method multiple times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@narcis-radu it is not going to be called to many times but several times definitely.
later after commerce.js, we will start adding maslibs feature to merch-card, merch-card-collection and other merch blocks as well.
merch being auto block it will be the first one to call this method and have the base url initialised, then the other blocks can use the value.
@3ch023 I'm tempted to add merch-card in this PR, thus we can give a better shape to the code, wdyt? cc: @npeltier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still not happy with the
baseUrl
approach since it doesn't follow the pattern used in other Milo blocks, but I don't think this is a blocker or reason to request changes. Thanks for addressing the important changes 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, something like
getMasLibs
setMasLibs
would be better. I'll give it a try tomorrow my morning.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't the method itself get the
hostname
andsearchParams
? What's the value of sending these from here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept
getMasBase
as a pure function so that it is easier to test it.Since we need to check whether
maslibs
param is present here andgetMasBase
only needshostname
andmaslibs
parameters, I limited it to those params.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will address this in a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi: @narcis-radu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider other STAGE environments like BACOM or Firefly ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaik BACOM doesn't have prices, and Firefly is not merch enabled yet.
This can be done later as part of merch enablement on Firefly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even with that, for now it's mostly for testing, and most of our pages are on cc/dc so i think we are good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a lint change? (fine with me, just clarifying)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also typo: undefinded :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hehe