-
Notifications
You must be signed in to change notification settings - Fork 5
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
FX-1738: only exclude collection hubs from the iOS app #27
Conversation
0d9692b
to
9a3dfdf
Compare
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.
Looks good 👍 Just one comment.
Note that for sequencing, we will want to do the following:
- Release Collections for iOS
- Wait a bit for users to upgrade (about a week, I'd say?)
- Merge this PR and deploy
apple-app-site-association.json
Outdated
"NOT *L3ZpZGVv*", | ||
"NOT *L25l*", | ||
"NOT *L25ld3Mv*", | ||
"NOT **", |
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 don't know why these keep showing up in the file, but we should remove NOT **
entries.
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 is a bug in the yarn build
script for this repo. Just got these extraneous NOT **
after running it.
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.
Yeah, I need to dig into that 😅
d3de470
to
ea3f67d
Compare
apple-app-site-association.json
Outdated
@@ -1,5 +1,5 @@ | |||
{ | |||
"activitycontinuation": { | |||
"activity ntinuation": { |
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 seems like an accidental deletion of some characters.
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.
Aww good catch!
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.
Just one quick comment, other than that looks good!
@@ -25,8 +25,6 @@ | |||
"NOT /2016-year-in-art", | |||
"NOT /venice-biennale*", | |||
"NOT /gender-equality*", | |||
"NOT /collections", |
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.
Did we build this index view in the app? I think we'll still want users to see the web version of this (but could totally be missing something).
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.
+1 I think we still want to keep this line for now.
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.
Good catch. I just corrected it.
* commits output of `yarn build`, but removing `NOT **` entries which aren't desirable. See [this GH comment][0]. [0]:#27 (comment)
🚀 PR was released in |
Addresses: FX-1738 and FX-1739
This remove the rule to exclude all collections pages from being shown in the iOS app
and scopes it down to just exclude department (hub) collections. I addresses some merge conflicts in this PR so I'd appreciate it the reviewers took extra care to look at that as well.Once this is merged we will need to update the dependencies that use this library:
*Updates to Sailthru requires you to notify Sailthru support and provide them with the updated
apple-app-site-association.json
file.