Bug 1891121 - Add experiment brief links to experiment and rollout tables#524
Bug 1891121 - Add experiment brief links to experiment and rollout tables#524sarahhjchung merged 7 commits intomainfrom
Conversation
✅ Deploy Preview for fxms-skylight ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
|
|
We need to request that the experimenter team do it (or put in a PR ourselves and ask for their review). |
sarahhjchung
force-pushed
the
bug-1891121
branch
from
19d3652 to
deed77f
Compare
dmose
left a comment
dmose
left a comment
There was a problem hiding this comment.
This is looking good! A couple of UX things that I think we want to tweak though:
- In all of them, I think the experiment slug wants to be indented to start in the same place as the human-readable experiment name, as it's visually a bit confusing as-is. That said, that would line the experiment info up with the branch info, so maybe we'd want to indent the branches more too. Play around a bit and see what looks reasonable.
- In the Mozilla VPN one, if it's easy to prevent wrapping (eg with
) a line-break just before the symbol, I think the visual presentation would be smoother.
| <svg | ||
| fill="currentColor" | ||
| fillOpacity="0.6" | ||
| viewBox="0 0 8 8" | ||
| className="inline h-[1.2rem] w-[1.2rem] px-1" | ||
| aria-hidden="true" | ||
| style={{ | ||
| marginInline: "0.1rem 0", | ||
| paddingBlock: "0 0.1rem", | ||
| overflow: "visible", | ||
| }} | ||
| > | ||
| <path d="m1.71278 0h.57093c.31531 0 .57092.255837.57092.571429 0 .315591-.25561.571431-.57092.571431h-.57093c-.31531 0-.57093.25583-.57093.57143v4.57142c0 .3156.25562.57143.57093.57143h4.56741c.31531 0 .57093-.25583.57093-.57143v-.57142c0-.3156.25561-.57143.57092-.57143.31532 0 .57093.25583.57093.57143v.57142c0 .94678-.76684 1.71429-1.71278 1.71429h-4.56741c-.945943 0-1.71278-.76751-1.71278-1.71429v-4.57142c0-.946778.766837-1.71429 1.71278-1.71429zm5.71629 0c.23083.0002686.43879.13963.52697.353143.02881.069172.04375.143342.04396.218286v2.857141c0 .31559-.25561.57143-.57093.57143-.31531 0-.57092-.25584-.57092-.57143v-1.47771l-1.88006 1.88171c-.14335.14855-.35562.20812-.55523.15583-.19962-.0523-.35551-.20832-.40775-.40811-.05225-.19979.00727-.41225.15569-.55572l1.88006-1.88171h-1.47642c-.31531 0-.57093-.25584-.57093-.571431 0-.315592.25562-.571429.57093-.571429z"></path> |
There was a problem hiding this comment.
Can you add an XXX comment both here and later in the file that we should switch to the Lucide Icon for an external link instead of maintaining our own? (I don't think we need to fix this now).
| "@auth0/nextjs-auth0": "^3.6.0", | ||
| "@looker/sdk-node": "25.4.0", | ||
| "@mozilla/nimbus-schemas": "^3001.0.0", | ||
| "@mozilla/nimbus-shared": "^2.5.2", |
There was a problem hiding this comment.
From reading https://github.com/mozilla/nimbus-shared, it sounds like we don't need nimbus-shared any more.
| const nimbusExperimentV7Schema = require("@mozilla/nimbus-schemas/schemas/NimbusExperimentV7.schema.json"); | ||
| type NimbusExperiment = typeof nimbusExperimentV7Schema.properties; | ||
| type DocumentationLink = | ||
| typeof nimbusExperimentV7Schema.properties.documentationLinks; |
There was a problem hiding this comment.
I think it would be helpful to put JSDoc comments here explaining why you're doing the type aliasing.
|
I played around with the spacing and indenting and concluded on this. 
I've made the user facing name experiment/rollout (ie. the bold text) to have the |
sarahhjchung
force-pushed
the
bug-1891121
branch
from
4b82d02 to
8d28da2
Compare
dmose
left a comment
dmose
left a comment
There was a problem hiding this comment.
This looks good to me. One small question to answer and/or address; I don't need to see this again. Thanks for making this happen as well as your patience! r=dmose
| surface: "Menu Messages", | ||
| tagColor: "bg-pink-300", | ||
| }, | ||
| menu_message: { |
There was a problem hiding this comment.
There are two different names for the same surface?
There was a problem hiding this comment.
Yes, I noticed there was a surface called "menu" in the live message table and some called "menu_message" in the experiments table. However, I don't see the experiment anymore so I can remove this for now and can create a new PR for it if it shows up again.
2 participants
Bug 1891121
Changes made:
RecipeInfonow includes anexperimentBriefLinkproperty with the url to experiment link Google docsFileTexticon from lucide-icons is now displayed next to the experiment/rollout name if an experiment brief link exists@mozilla/nimbus-schemashas been imported and replaces theNimbusExperimenttype used from@mozilla/nimbus-shared