-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: ACI Notification Banners #23256
Conversation
…com/cypress-io/cypress into mikep/22933-aci-smart-notifications
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ FlakinessThis comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
* Add utm params to login & create org links * Fix org refresh on create * Fix styling of record command
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 a basic review and ask a few questions, sparked some conversation... I will give it a proper test now and report back.
@@ -96,7 +96,7 @@ | |||
</Button> | |||
</Alert> | |||
<Alert | |||
v-if="showProjectRequestAccess" | |||
v-else-if="showProjectRequestAccess" |
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 think many v-else-if combination is fine, but one thing I started using in my personal projects lately is v-switch
, I wonder if it would be useful in Cypress, too: https://www.npmjs.com/package/@lmiller1990/v-switch
The source code is only about 10 lines, we could just copy-paste and register as a global component.
Still reviewing code and tests but wanted to share that functionality all works for me! @pstakoun Product feedback - I wasn't quite expecting the "guided tour" feeling where if I don't dismiss the banner, its content changes on the next step - eg, "Login" content -> "Connect" content -> "Record" content. UX wise the internal content of a banner changing like this feels like it could use some sort of "journey" UI ("Step 1 of 3") or some element that visibly "progresses" as well as simply the text content being updated. I think the "guided" feeling is good & leaning into it a bit could be a future enhancement. It also feels like a temporary banner might not be the exact right UI element to reflect this, it's a bit counterintuitive to me that the contents of a dismissable banner change over time, it's different to all our other banners that look similar. |
@marktnoonan totally agree with you and that feedback resonates with me. I think banners are not the form-factor that this experience will be in the near future - an onboarding checklist similar to that in the Dashboard is also under consideration. |
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 tested each of the banner situations with staging. Each one worked correctly for me. One thing was that in order to get into the state where I was logged in but wasn't in an org, I had to delete my existing org. Not sure how a user would get into this state since I think you need to be in an org to complete the login flow, but good that we're handling that either way.
Nice work 🎉 I'm excited to see these banners get more styled in the future!
@marktnoonan I don't think we should add custom styling for these at this time. From a UX perspective the app has a hard minimum width, and I think it would look kinda funky for parts of the UI to try to be responsive while the majority overflows. I think we should be more responsive, but it should be tackled holistically so there is a consistent experience |
@@ -30,7 +30,7 @@ | |||
</p> | |||
</Alert> | |||
<Alert | |||
v-if="showFetchError" | |||
v-else-if="showFetchError" | |||
v-model="showFetchError" |
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.
Hmm. The combination of v-if
and v-model
pointing to the same value seems like a hint that v-model
is not doing what we want and hiding/showing the Alert on its own?
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 was an attempt to simplify the "only show one banner" concern within SpecsListBanners.vue
. Currently, showFetchError
just cares about whether the conditions for its display are met independently, but if we remove the v-if/v-else-if
then I would need to add conditions to check whether any higher-priority banners are shown when determining the model value. This felt like a cleaner approach
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 that makes sense, at least for this iteration. I know more is coming 👍
/> | ||
<LoginBanner | ||
v-else-if="showLoginBanner" | ||
v-model="showLoginBanner" |
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.
Similar comment here. Seems like we could push the if
inside these components (or use a wrapper) and drive the presence/absence of these things from the state being tracked via the v-model
.
|
||
cy.get('a') | ||
.should('have.attr', 'href') | ||
.and('contain', linkHref) |
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 assert the specPage.banners.Join or create an organization in Cypress Dashboard to access your projects and recorded test runs
here? Since grabbing that content is also a responsibility of this component.
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.
No actual blockers but requesting changes to discuss merge timing.
it('should render expected content', () => { | ||
cy.mount({ render: () => <LoginBanner modelValue={true} /> }) | ||
|
||
cy.percySnapshot() |
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.
Similar to an earlier comment, I'd like this test to fail if the content were missing. Percy will catch this too but is a lot slower/sometimes gets ignored.
Or maybe we have an e2e test or higher level component test that checks the contents, and I haven't see it yet.
@@ -378,6 +378,10 @@ describe('<HeaderBarContent />', { viewportWidth: 1000, viewportHeight: 750 }, ( | |||
}, | |||
render: (gqlVal) => <div class="border-current border-1 h-700px resize overflow-auto"><HeaderBarContent gql={gqlVal} show-browsers={true} allowAutomaticPromptOpen={true} /></div>, | |||
}) | |||
|
|||
cy.tick(2000) |
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.
Could we add a comment with the intention behind this tick? Would tests fail if we removed it?
const isOpenDelayElapsed = ref(false) | ||
|
||
onMounted(() => { | ||
setTimeout(() => isOpenDelayElapsed.value = true, 2000) |
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.
nice, this makes sense of the tick
in the test code for mounting.
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.
Sidenote: we could simplify a little and remove the mounted hook updating the ref with useTimeout
(https://vueuse.org/shared/usetimeout/)
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've left a few comments that are mostly nits, I think it worth adding assertions to cover the remaining new content in en-US.json
in the component specs for the components that render those strings though.
Will leave my change request up for that reason, but @mike-plummer nothing else is blocking (and feel free to point of if I'm missed something about the content).
* Add test assertions for banner text content * Add comment to cy.tick
Dashboard Integration Smart Login Notification #22933
Dashboard Integration Smart Organization Creation Notification #22934
Dashboard Integration banner state for "no projectId"/"connect your project" state #22798
Dashboard Integration Smart Record Run Notification #22935
User facing changelog
Additional details
Four main areas here:
TrackedBanner
component to allow reuse of storing "last shown" and "dismissed" info for bannersSpecsListBanners
component - this is currently serving as a "banner controller" of sorts to control which banners to display and when (a refinement of this construct is coming in future work related to A/B testing of banner content)Steps to test
Note: To fully test it will be necessary to either create a new user or have an existing user removed from all organizations. You may want to create a new "throwaway" user rather than remove yourself from the shared Cypress org in staging.
Note: launch with CYPRESS_INTERNAL_ENV=staging for staging.
state.json
file. On Mac this is stored in a project-specific directory under~/Library/Application Support/Cypress/cy/staging/projects/
state.json
to have afirstOpened
value more than 4 days in the past:Date.now() - 8.64e+7 * 4
state.json
, observe an entry for the login banner has been created. Remove it, and reopen the project.--component
flag for CT, valid record key) and can be copy-pastedHow has the user experience changed?
Login Banner
Login.mov
Create Org Banner
CreateOrg.mov
Connect Project Banner
ConnectProject.mov
Record Banner
Record.mov
PR Tasks
cypress-documentation
?type definitions
?