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

feat: add show only framed works filter #15108

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MounirDhahri
Copy link
Member

The type of this PR is: feat

This PR adds show only framed works filter to artwork filters

This PR is a follow-up to artsy/eigen#11435, artsy/metaphysics#6369 and https://github.com/artsy/gravity/pull/18481

Screen.Recording.2025-01-23.at.15.18.50.mov

Copy link

relativeci bot commented Jan 23, 2025

#1588 Bundle Size — 8.97MiB (-0.32%).

4b6aed3(current) vs 41ddd7f main#1587(baseline)

Warning

Bundle contains 14 duplicate packages – View duplicate packages

Bundle metrics  Change 5 changes Improvement 2 improvements
                 Current
#1588
     Baseline
#1587
Improvement  Initial JS 3.65MiB(-0.05%) 3.65MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 53.74% 43.59%
No change  Chunks 104 104
No change  Assets 107 107
Change  Modules 5831(-0.39%) 5854
Improvement  Duplicate Modules 515(-3.01%) 531
Change  Duplicate Code 3.97%(-2.7%) 4.08%
No change  Packages 266 266
No change  Duplicate Packages 13 13
Bundle size by type  Change 2 changes Regression 1 regression Improvement 1 improvement
                 Current
#1588
     Baseline
#1587
Improvement  JS 8.83MiB (-0.33%) 8.86MiB
Regression  Other 145.45KiB (+0.15%) 145.23KiB

Bundle analysis reportBranch feat/support-framed-filteringProject dashboard


Generated by RelativeCIDocumentationReport issue

Copy link
Member

@anandaroop anandaroop left a comment

Choose a reason for hiding this comment

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

Looks legit, nice job!

// but NOT as an alert criterion, so we remove it.
// (Alerts, by definition, stipulate forSale=true
// when they are created in Gravity.)
delete criteria.forSale
delete criteria.framed
Copy link
Member

Choose a reason for hiding this comment

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

thought (non-blocking): I think this makes sense for now. Maybe a product decision about whether this would be a desirable alert criteria, but we can always follow up on that.

Copy link
Contributor

@olerichter00 olerichter00 left a comment

Choose a reason for hiding this comment

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

🌟

Comment on lines +26 to +28
const enableShowOnlyFramedArtworksFilter = useFeatureFlag(
"onyx_only_framed_artworks_filter",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need the feature flag? In case something goes wrong, we cand simply roll back the feature.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW I'm in favor of the feature flag. Gives us the ability to do more fine-grained QA (in staging, or per user) before launching. I love the velocity of Hackathon but I appreciate rolling things out intentionally like this 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

True, the feature flag gives more flexibility for the release (and frees us from having to follow the bug-free programming principle 😄).

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.

3 participants