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

ref(whats-new): Revamp "Whats New" #76818

Merged
merged 23 commits into from
Sep 6, 2024
Merged

ref(whats-new): Revamp "Whats New" #76818

merged 23 commits into from
Sep 6, 2024

Conversation

priscilawebdev
Copy link
Member

@priscilawebdev priscilawebdev commented Sep 2, 2024 •
edited
Loading

Before
image
image

After

Screenshot 2024-09-05 at 22 34 58 Screenshot 2024-09-05 at 22 34 26

closes #77055

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Sep 2, 2024
@codecov Codecov
Copy link

codecov bot commented Sep 2, 2024 •
edited
Loading

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
7734 1 7733 0
View the top 1 failed tests by shortest run time 
Broadcasts renders item with media Broadcasts renders item with media
Stack Traces | 0.054s run time 
TestingLibraryElementError: Unable to find an element with the text: blog post. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

Ignored nodes: comments, script, style
...
    at Object.getElementError (.../sentry/sentry/node_modules/@.../dom/dist/config.js:37:19)
    at .../sentry/sentry/node_modules/@.../dom/dist/query-helpers.js:76:38
    at .../sentry/sentry/node_modules/@.../dom/dist/query-helpers.js:52:17
    at .../sentry/sentry/node_modules/@.../dom/dist/query-helpers.js:95:19
    at Object.<anonymous> (.../components/sidebar/broadcasts.spec.tsx:66:40)

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@priscilawebdev priscilawebdev marked this pull request as ready for review September 3, 2024 14:28
@priscilawebdev priscilawebdev requested review from roggenkemper and a team September 3, 2024 14:28
@priscilawebdev priscilawebdev marked this pull request as draft September 5, 2024 08:36
@priscilawebdev priscilawebdev marked this pull request as ready for review September 5, 2024 13:22
roggenkemper
roggenkemper reviewed Sep 5, 2024
View reviewed changes
static/app/components/sidebar/utils.tsx Outdated

export const isDone = (task: OnboardingTaskStatus) =>
task.status === 'complete' || task.status === 'skipped';

// To be passed as the `source` parameter in router navigation state
// e.g. {pathname: '/issues/', state: {source: `sidebar`}}
export const SIDEBAR_NAVIGATION_SOURCE = 'sidebar';

export function hasWhatIsNewRevampFeature(organization: Organization) {
return organization.features.includes('what-is-new-revamp');
Copy link
Member

Choose a reason for hiding this comment

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

two nits here - would prefer we call this whats-new-revamp if the flag hasn't been created yet. second, do we really need this util function ? i feel like since it is just a feature flag check we could have it be const hasWhatsNewRevamp = organization.features.includes()...;

Copy link
Member Author

Choose a reason for hiding this comment

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

yes initially I thought we would use it in more different places but it makes sense

Copy link
Member Author

@priscilawebdev priscilawebdev Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

oh and the feature flag has already been created

roggenkemper
roggenkemper reviewed Sep 5, 2024
View reviewed changes
static/app/types/system.tsx Outdated
};
/**
* Category of the broadcast.
* Synced with https://github.com/getsentry/sentry/blob/923a65508912c3e181e1c70cbdf076b7b956aa90/src/sentry/models/broadcast.py#L14
Copy link
Member

Choose a reason for hiding this comment

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

can we change the branch here to be master: https://github.com/getsentry/sentry/blob/master/src/sentry/models/broadcast.py

Copy link
Member Author

@priscilawebdev priscilawebdev Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I tried that a couple of times, but always when I clicked on the copy button I got a blob/code never mind... copying the URL works too

@priscilawebdev priscilawebdev changed the title ref(whats-new): Add improvements ref(whats-new): Revamp "Whats New" Sep 6, 2024
@priscilawebdev priscilawebdev merged commit 396d044 into master Sep 6, 2024
45 checks passed
@priscilawebdev priscilawebdev deleted the priscila/ref/whats-new/improvements branch September 6, 2024 11:13
c298lee pushed a commit that referenced this pull request Sep 10, 2024
@priscilawebdev @c298lee
ref(whats-new): Revamp "Whats New" (#76818)
f3a0456
@sentry-io Sentry.io
Copy link

sentry-io bot commented Sep 13, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ TypeError: Cannot destructure property 'basename' of 'React__namespace.useContext(...)' as it is null. render(reactTestingLibrary.tsx) View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matejminar matejminar matejminar approved these changes

@roggenkemper roggenkemper Awaiting requested review from roggenkemper

Assignees
No one assigned
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Revamp "Whats New"
3 participants
@priscilawebdev @matejminar @roggenkemper