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.

Sign up for GitHub

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

Jump to bottom

feat(aci): use feature check for aci #1508

Merged
merged 4 commits into from
Oct 4, 2023
Merged

feat(aci): use feature check for aci #1508

merged 4 commits into from
Oct 4, 2023

Conversation

arturfrombox
Copy link
Contributor

@arturfrombox arturfrombox commented Sep 25, 2023 •
edited
Loading

This pr introduces changes to cover ACI codebase with feature checking

@arturfrombox arturfrombox requested a review from a team as a code owner September 25, 2023 13:03
ahorowitz123
ahorowitz123 reviewed Sep 25, 2023
View reviewed changes
src/lib/Preview.js Outdated Show resolved Hide resolved
ahorowitz123
ahorowitz123 reviewed Sep 25, 2023
View reviewed changes
src/lib/Preview.js Outdated
...this.previewOptions,
features: { ...this.previewOptions.features, advancedContentInsights: options },
};
this.viewer.pageTracker.updateOptions(this.previewOptions.features.advancedContentInsights);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just use options as the argument to the function here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you explain why we should do so ?

Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: I also found it a bit difficult to grasp having both options and this.options

Copy link
Contributor

@ahorowitz123 ahorowitz123 Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

nit: I still find this confusing as well. Why not just call this.viewer.pageTracker.updateOptions(newOptions) here? It's much easier to read in my option and stays more consistent with the rest of this code block

jstoffan
jstoffan reviewed Sep 25, 2023
View reviewed changes
Copy link
Collaborator

@jstoffan jstoffan left a comment

Choose a reason for hiding this comment

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

What's benefit of these changes if we already have a configuration object for these options? We appear to be increasing complexity at every layer.

@arturfrombox
Copy link
Contributor Author

arturfrombox commented Sep 26, 2023 •
edited
Loading

What's benefit of these changes if we already have a configuration object for these options? We appear to be increasing complexity at every layer.

It was one of PMAI's and intention behind this change was to stick to "feature checking" mechanism implemented for Preview and to hide rest of ACI codebase under the feature flip.

@arturfrombox
Copy link
Contributor Author

@ahorowitz123 @jstoffan thanks for your comments. I updated this PR

ahorowitz123
ahorowitz123 previously approved these changes Oct 3, 2023
View reviewed changes
@arturfrombox arturfrombox merged commit 4616da7 into box:master Oct 4, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahorowitz123 ahorowitz123 ahorowitz123 approved these changes

@loonskai loonskai loonskai approved these changes

@jstoffan jstoffan Awaiting requested review from jstoffan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arturfrombox @jstoffan @ahorowitz123 @loonskai