feat(events-v2) Add a event graph to the event modal - #13568

Merged
markstory merged 14 commits into
masterfrom
sen-716-modal-chart
Jun 11, 2019
Merged

feat(events-v2) Add a event graph to the event modal#13568
markstory merged 14 commits into
masterfrom
sen-716-modal-chart

Conversation

@markstory

@markstorymarkstory commented Jun 6, 2019

Copy link
Copy Markdown
Member

Show an event graph for grouped views. The grouped view mode requires showing a single event + grouped data above the single event view. I've implemented a stripped down line graph as we don't need release markers or previous period markers on this graph. Additionally we'll need special click handling that navigates to other events in the series.

TODO

  • Add UI integration jest tests

Refs SEN-716

@markstory
markstory requested a review from a teamJune 6, 2019 15:02

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How do you specify multiple groupIds?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Right now I don't have use-case where I need more than one group, I'll expand the proptype to allow for future scenarios though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The _ is not necessary here

@markstory

Copy link
Copy Markdown
MemberAuthor

Screen Shot 2019-06-06 at 1 50 28 PM

Screenshot of what it looks like in action

@markstory
markstoryforce-pushed the sen-716-modal-chart branch from f806d94 to cc390b3CompareJune 7, 2019 14:55
@markstory
markstory changed the base branch from sen-716-endpoint-last-event to feat/org-event-detailsJune 7, 2019 14:55
@markstory
markstory requested a review from lynnagaraJune 7, 2019 14:56
@markstory

Copy link
Copy Markdown
MemberAuthor

@lynnagara I've rebased this on top of your branch and switched to using the /latest endpoint when getting event information for grouped views.

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: PropTypes.string,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could have a proptype that enforces the eventSlug format

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sure!

Comment threadsrc/sentry/api/urls.py Outdated
@markstory
markstory changed the base branch from feat/org-event-details to masterJune 10, 2019 17:53
Show an event graph for grouped views. The grouped view mode requires
showing a single event + grouped data above the single event view. I've
implemented a stripped down line graph as we don't need release markers
or previous period markers on this graph. Additionally we'll need
special click handling that navigates to other events in the series.
Refs SEN-716
Use the new /latest endpoint in the grouped modal view. This pushes the
complexity of finding the most recent event into the API where it should
be.
Lyn prefers to keep the URL parameter `:` separated.
By creating a combined query parameter we have fewer state bits we need
to manage together. Instead we can focus on the valid/invalid value
states.
In grouped views we want to show pagination buttons to navigate between
the events that match the current search filters including the query
string. By creating URLs that use eventSlug and the current search
criteria we'll end up at the right events.
Refs SEN-715
@markstory
markstoryforce-pushed the sen-716-modal-chart branch from 7681f97 to 36f6b86CompareJune 10, 2019 19:54
@markstory

Copy link
Copy Markdown
MemberAuthor

I've included the pagination changes in this pull request now as well. The top half of the event modal looks like:

Screen Shot 2019-06-10 at 11 50 47 AM

@markstory
markstory requested review from a team and lynnagaraJune 11, 2019 14:44

const slugValidator = function(props, propName, componentName) {
const value = props[propName];
if (value && !/^(?:[^:]+:)?(?:[^:]+):(?:[a-f0-9]+|latest|oldest)$/.test(value)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:nice:regex:

It'd help to have a comment with what a valid slug looks like. Are special characters (non alphanumerics) allowed in the first two groups?

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: slugValidator,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to validate these at runtime as well? (I guess receiving a 404 makes sense)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think the 404 we display in the modal could use some work, but I can do that separately.

const ModalLineGraph = props => {
const {api, groupId, organization, location, selection} = props;

const isUtc = selection.datetime.utc;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So if selection.datetime.utc is null it won't necessarily mean that the user doesn't want it to be formatted UTC, it just means that they haven't use the UTC override in the global selection header. I think we'd want to fallback to user's timezone in ConfigStore.

Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
@markstory
markstory merged commit dc4fbea into masterJun 11, 2019
@markstory
markstory deleted the sen-716-modal-chart branch June 11, 2019 20:50
jan-auer added a commit that referenced this pull request Jun 12, 2019
* master: (102 commits)
fix(symbolicator): Tests are no longer allowed failures (#13566)
fix(yarn): Fix yarn path in distutils (#13647)
ref(ui): Remove OrganizationState from a bunch of places (#13646)
ref(ui): Remove crufty global exports (#13578)
feat(events-v2): Page loader improvements (#13645)
fix: Query the right set of issues when given no set of envs for a given first release (#13551)
feat(ui): update incidents icon (#13622)
fix(ui): remove rate-limited values from graph when no events are present (#13624)
feat(events-v2) Add a event graph to the event modal (#13568)
fix(store): Check project.status is VISIBLE before attempting to process
perf(snuba): Limit SnubaEvent.get_event to return the first result only. (#13644)
tests(percy): Fix Incident status item dynamic time (#13640)
ref(sentry10): Clean up action creators (#13636)
ref(app-platform): Backfill IntegrationFeature table (#13548)
ref(utils): Remove default / reexports from utils.jsx (#13581)
feat(sentry10): Remove the ability to set a default environment (#13637)
feat(api): Add url unfurls for incidents to slack integration (SEN-566)
build(yarn): Upgrade yarn to 1.16.0, embed in repo (#13569)
feat(api): Allow passing group_ids to `OrganizationGroupIndexEndpoint` for bulk fetching groups (SEN-701)
feat(ui): Adds related issues to Incidents sidebar [SEN-695] (#13617)
...
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@markstory@billyvg@lynnagara
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

feat(events-v2) Add a event graph to the event modal - #13568

Merged
markstory merged 14 commits into
masterfrom
sen-716-modal-chart
Jun 11, 2019
Merged

feat(events-v2) Add a event graph to the event modal#13568
markstory merged 14 commits into
masterfrom
sen-716-modal-chart

Conversation

@markstory

@markstorymarkstory commented Jun 6, 2019

Copy link
Copy Markdown
Member

Show an event graph for grouped views. The grouped view mode requires showing a single event + grouped data above the single event view. I've implemented a stripped down line graph as we don't need release markers or previous period markers on this graph. Additionally we'll need special click handling that navigates to other events in the series.

TODO

  • Add UI integration jest tests

Refs SEN-716

@markstory
markstory requested a review from a teamJune 6, 2019 15:02

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How do you specify multiple groupIds?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Right now I don't have use-case where I need more than one group, I'll expand the proptype to allow for future scenarios though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The _ is not necessary here

@markstory

Copy link
Copy Markdown
MemberAuthor

Screen Shot 2019-06-06 at 1 50 28 PM

Screenshot of what it looks like in action

@markstory
markstoryforce-pushed the sen-716-modal-chart branch from f806d94 to cc390b3CompareJune 7, 2019 14:55
@markstory
markstory changed the base branch from sen-716-endpoint-last-event to feat/org-event-detailsJune 7, 2019 14:55
@markstory
markstory requested a review from lynnagaraJune 7, 2019 14:56
@markstory

Copy link
Copy Markdown
MemberAuthor

@lynnagara I've rebased this on top of your branch and switched to using the /latest endpoint when getting event information for grouped views.

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: PropTypes.string,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could have a proptype that enforces the eventSlug format

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sure!

Comment threadsrc/sentry/api/urls.py Outdated
@markstory
markstory changed the base branch from feat/org-event-details to masterJune 10, 2019 17:53
Show an event graph for grouped views. The grouped view mode requires
showing a single event + grouped data above the single event view. I've
implemented a stripped down line graph as we don't need release markers
or previous period markers on this graph. Additionally we'll need
special click handling that navigates to other events in the series.
Refs SEN-716
Use the new /latest endpoint in the grouped modal view. This pushes the
complexity of finding the most recent event into the API where it should
be.
Lyn prefers to keep the URL parameter `:` separated.
By creating a combined query parameter we have fewer state bits we need
to manage together. Instead we can focus on the valid/invalid value
states.
In grouped views we want to show pagination buttons to navigate between
the events that match the current search filters including the query
string. By creating URLs that use eventSlug and the current search
criteria we'll end up at the right events.
Refs SEN-715
@markstory
markstoryforce-pushed the sen-716-modal-chart branch from 7681f97 to 36f6b86CompareJune 10, 2019 19:54
@markstory

Copy link
Copy Markdown
MemberAuthor

I've included the pagination changes in this pull request now as well. The top half of the event modal looks like:

Screen Shot 2019-06-10 at 11 50 47 AM

@markstory
markstory requested review from a team and lynnagaraJune 11, 2019 14:44

const slugValidator = function(props, propName, componentName) {
const value = props[propName];
if (value && !/^(?:[^:]+:)?(?:[^:]+):(?:[a-f0-9]+|latest|oldest)$/.test(value)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:nice:regex:

It'd help to have a comment with what a valid slug looks like. Are special characters (non alphanumerics) allowed in the first two groups?

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: slugValidator,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to validate these at runtime as well? (I guess receiving a 404 makes sense)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think the 404 we display in the modal could use some work, but I can do that separately.

const ModalLineGraph = props => {
const {api, groupId, organization, location, selection} = props;

const isUtc = selection.datetime.utc;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So if selection.datetime.utc is null it won't necessarily mean that the user doesn't want it to be formatted UTC, it just means that they haven't use the UTC override in the global selection header. I think we'd want to fallback to user's timezone in ConfigStore.

Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
@markstory
markstory merged commit dc4fbea into masterJun 11, 2019
@markstory
markstory deleted the sen-716-modal-chart branch June 11, 2019 20:50
jan-auer added a commit that referenced this pull request Jun 12, 2019
* master: (102 commits)
fix(symbolicator): Tests are no longer allowed failures (#13566)
fix(yarn): Fix yarn path in distutils (#13647)
ref(ui): Remove OrganizationState from a bunch of places (#13646)
ref(ui): Remove crufty global exports (#13578)
feat(events-v2): Page loader improvements (#13645)
fix: Query the right set of issues when given no set of envs for a given first release (#13551)
feat(ui): update incidents icon (#13622)
fix(ui): remove rate-limited values from graph when no events are present (#13624)
feat(events-v2) Add a event graph to the event modal (#13568)
fix(store): Check project.status is VISIBLE before attempting to process
perf(snuba): Limit SnubaEvent.get_event to return the first result only. (#13644)
tests(percy): Fix Incident status item dynamic time (#13640)
ref(sentry10): Clean up action creators (#13636)
ref(app-platform): Backfill IntegrationFeature table (#13548)
ref(utils): Remove default / reexports from utils.jsx (#13581)
feat(sentry10): Remove the ability to set a default environment (#13637)
feat(api): Add url unfurls for incidents to slack integration (SEN-566)
build(yarn): Upgrade yarn to 1.16.0, embed in repo (#13569)
feat(api): Allow passing group_ids to `OrganizationGroupIndexEndpoint` for bulk fetching groups (SEN-701)
feat(ui): Adds related issues to Incidents sidebar [SEN-695] (#13617)
...
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@markstory@billyvg@lynnagara
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(events-v2) Add a event graph to the event modal - #13568

Merged
markstory merged 14 commits into
masterfrom
sen-716-modal-chart
Jun 11, 2019
Merged

feat(events-v2) Add a event graph to the event modal#13568
markstory merged 14 commits into
masterfrom
sen-716-modal-chart

Conversation

@markstory

@markstorymarkstory commented Jun 6, 2019

Copy link
Copy Markdown
Member

Show an event graph for grouped views. The grouped view mode requires showing a single event + grouped data above the single event view. I've implemented a stripped down line graph as we don't need release markers or previous period markers on this graph. Additionally we'll need special click handling that navigates to other events in the series.

TODO

  • Add UI integration jest tests

Refs SEN-716

@markstory
markstory requested a review from a teamJune 6, 2019 15:02

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How do you specify multiple groupIds?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Right now I don't have use-case where I need more than one group, I'll expand the proptype to allow for future scenarios though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The _ is not necessary here

@markstory

Copy link
Copy Markdown
MemberAuthor

Screen Shot 2019-06-06 at 1 50 28 PM

Screenshot of what it looks like in action

@markstory
markstoryforce-pushed the sen-716-modal-chart branch from f806d94 to cc390b3CompareJune 7, 2019 14:55
@markstory
markstory changed the base branch from sen-716-endpoint-last-event to feat/org-event-detailsJune 7, 2019 14:55
@markstory
markstory requested a review from lynnagaraJune 7, 2019 14:56
@markstory

Copy link
Copy Markdown
MemberAuthor

@lynnagara I've rebased this on top of your branch and switched to using the /latest endpoint when getting event information for grouped views.

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: PropTypes.string,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could have a proptype that enforces the eventSlug format

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sure!

Comment threadsrc/sentry/api/urls.py Outdated
@markstory
markstory changed the base branch from feat/org-event-details to masterJune 10, 2019 17:53
Show an event graph for grouped views. The grouped view mode requires
showing a single event + grouped data above the single event view. I've
implemented a stripped down line graph as we don't need release markers
or previous period markers on this graph. Additionally we'll need
special click handling that navigates to other events in the series.
Refs SEN-716
Use the new /latest endpoint in the grouped modal view. This pushes the
complexity of finding the most recent event into the API where it should
be.
Lyn prefers to keep the URL parameter `:` separated.
By creating a combined query parameter we have fewer state bits we need
to manage together. Instead we can focus on the valid/invalid value
states.
In grouped views we want to show pagination buttons to navigate between
the events that match the current search filters including the query
string. By creating URLs that use eventSlug and the current search
criteria we'll end up at the right events.
Refs SEN-715
@markstory
markstoryforce-pushed the sen-716-modal-chart branch from 7681f97 to 36f6b86CompareJune 10, 2019 19:54
@markstory

Copy link
Copy Markdown
MemberAuthor

I've included the pagination changes in this pull request now as well. The top half of the event modal looks like:

Screen Shot 2019-06-10 at 11 50 47 AM

@markstory
markstory requested review from a team and lynnagaraJune 11, 2019 14:44

const slugValidator = function(props, propName, componentName) {
const value = props[propName];
if (value && !/^(?:[^:]+:)?(?:[^:]+):(?:[a-f0-9]+|latest|oldest)$/.test(value)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:nice:regex:

It'd help to have a comment with what a valid slug looks like. Are special characters (non alphanumerics) allowed in the first two groups?

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: slugValidator,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to validate these at runtime as well? (I guess receiving a 404 makes sense)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think the 404 we display in the modal could use some work, but I can do that separately.

const ModalLineGraph = props => {
const {api, groupId, organization, location, selection} = props;

const isUtc = selection.datetime.utc;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So if selection.datetime.utc is null it won't necessarily mean that the user doesn't want it to be formatted UTC, it just means that they haven't use the UTC override in the global selection header. I think we'd want to fallback to user's timezone in ConfigStore.

Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
@markstory
markstory merged commit dc4fbea into masterJun 11, 2019
@markstory
markstory deleted the sen-716-modal-chart branch June 11, 2019 20:50
jan-auer added a commit that referenced this pull request Jun 12, 2019
* master: (102 commits)
fix(symbolicator): Tests are no longer allowed failures (#13566)
fix(yarn): Fix yarn path in distutils (#13647)
ref(ui): Remove OrganizationState from a bunch of places (#13646)
ref(ui): Remove crufty global exports (#13578)
feat(events-v2): Page loader improvements (#13645)
fix: Query the right set of issues when given no set of envs for a given first release (#13551)
feat(ui): update incidents icon (#13622)
fix(ui): remove rate-limited values from graph when no events are present (#13624)
feat(events-v2) Add a event graph to the event modal (#13568)
fix(store): Check project.status is VISIBLE before attempting to process
perf(snuba): Limit SnubaEvent.get_event to return the first result only. (#13644)
tests(percy): Fix Incident status item dynamic time (#13640)
ref(sentry10): Clean up action creators (#13636)
ref(app-platform): Backfill IntegrationFeature table (#13548)
ref(utils): Remove default / reexports from utils.jsx (#13581)
feat(sentry10): Remove the ability to set a default environment (#13637)
feat(api): Add url unfurls for incidents to slack integration (SEN-566)
build(yarn): Upgrade yarn to 1.16.0, embed in repo (#13569)
feat(api): Allow passing group_ids to `OrganizationGroupIndexEndpoint` for bulk fetching groups (SEN-701)
feat(ui): Adds related issues to Incidents sidebar [SEN-695] (#13617)
...
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@markstory@billyvg@lynnagara
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(events-v2) Add a event graph to the event modal - #13568

Merged
markstory merged 14 commits into
masterfrom
sen-716-modal-chart
Jun 11, 2019
Merged

feat(events-v2) Add a event graph to the event modal#13568
markstory merged 14 commits into
masterfrom
sen-716-modal-chart

Conversation

@markstory

@markstorymarkstory commented Jun 6, 2019

Copy link
Copy Markdown
Member

Show an event graph for grouped views. The grouped view mode requires showing a single event + grouped data above the single event view. I've implemented a stripped down line graph as we don't need release markers or previous period markers on this graph. Additionally we'll need special click handling that navigates to other events in the series.

TODO

  • Add UI integration jest tests

Refs SEN-716

@markstory
markstory requested a review from a teamJune 6, 2019 15:02

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How do you specify multiple groupIds?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Right now I don't have use-case where I need more than one group, I'll expand the proptype to allow for future scenarios though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The _ is not necessary here

@markstory

Copy link
Copy Markdown
MemberAuthor

Screen Shot 2019-06-06 at 1 50 28 PM

Screenshot of what it looks like in action

@markstory
markstoryforce-pushed the sen-716-modal-chart branch from f806d94 to cc390b3CompareJune 7, 2019 14:55
@markstory
markstory changed the base branch from sen-716-endpoint-last-event to feat/org-event-detailsJune 7, 2019 14:55
@markstory
markstory requested a review from lynnagaraJune 7, 2019 14:56
@markstory

Copy link
Copy Markdown
MemberAuthor

@lynnagara I've rebased this on top of your branch and switched to using the /latest endpoint when getting event information for grouped views.

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: PropTypes.string,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could have a proptype that enforces the eventSlug format

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sure!

Comment threadsrc/sentry/api/urls.py Outdated
@markstory
markstory changed the base branch from feat/org-event-details to masterJune 10, 2019 17:53
Show an event graph for grouped views. The grouped view mode requires
showing a single event + grouped data above the single event view. I've
implemented a stripped down line graph as we don't need release markers
or previous period markers on this graph. Additionally we'll need
special click handling that navigates to other events in the series.
Refs SEN-716
Use the new /latest endpoint in the grouped modal view. This pushes the
complexity of finding the most recent event into the API where it should
be.
Lyn prefers to keep the URL parameter `:` separated.
By creating a combined query parameter we have fewer state bits we need
to manage together. Instead we can focus on the valid/invalid value
states.
In grouped views we want to show pagination buttons to navigate between
the events that match the current search filters including the query
string. By creating URLs that use eventSlug and the current search
criteria we'll end up at the right events.
Refs SEN-715
@markstory
markstoryforce-pushed the sen-716-modal-chart branch from 7681f97 to 36f6b86CompareJune 10, 2019 19:54
@markstory

Copy link
Copy Markdown
MemberAuthor

I've included the pagination changes in this pull request now as well. The top half of the event modal looks like:

Screen Shot 2019-06-10 at 11 50 47 AM

@markstory
markstory requested review from a team and lynnagaraJune 11, 2019 14:44

const slugValidator = function(props, propName, componentName) {
const value = props[propName];
if (value && !/^(?:[^:]+:)?(?:[^:]+):(?:[a-f0-9]+|latest|oldest)$/.test(value)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:nice:regex:

It'd help to have a comment with what a valid slug looks like. Are special characters (non alphanumerics) allowed in the first two groups?

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: slugValidator,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to validate these at runtime as well? (I guess receiving a 404 makes sense)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think the 404 we display in the modal could use some work, but I can do that separately.

const ModalLineGraph = props => {
const {api, groupId, organization, location, selection} = props;

const isUtc = selection.datetime.utc;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So if selection.datetime.utc is null it won't necessarily mean that the user doesn't want it to be formatted UTC, it just means that they haven't use the UTC override in the global selection header. I think we'd want to fallback to user's timezone in ConfigStore.

Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
@markstory
markstory merged commit dc4fbea into masterJun 11, 2019
@markstory
markstory deleted the sen-716-modal-chart branch June 11, 2019 20:50
jan-auer added a commit that referenced this pull request Jun 12, 2019
* master: (102 commits)
fix(symbolicator): Tests are no longer allowed failures (#13566)
fix(yarn): Fix yarn path in distutils (#13647)
ref(ui): Remove OrganizationState from a bunch of places (#13646)
ref(ui): Remove crufty global exports (#13578)
feat(events-v2): Page loader improvements (#13645)
fix: Query the right set of issues when given no set of envs for a given first release (#13551)
feat(ui): update incidents icon (#13622)
fix(ui): remove rate-limited values from graph when no events are present (#13624)
feat(events-v2) Add a event graph to the event modal (#13568)
fix(store): Check project.status is VISIBLE before attempting to process
perf(snuba): Limit SnubaEvent.get_event to return the first result only. (#13644)
tests(percy): Fix Incident status item dynamic time (#13640)
ref(sentry10): Clean up action creators (#13636)
ref(app-platform): Backfill IntegrationFeature table (#13548)
ref(utils): Remove default / reexports from utils.jsx (#13581)
feat(sentry10): Remove the ability to set a default environment (#13637)
feat(api): Add url unfurls for incidents to slack integration (SEN-566)
build(yarn): Upgrade yarn to 1.16.0, embed in repo (#13569)
feat(api): Allow passing group_ids to `OrganizationGroupIndexEndpoint` for bulk fetching groups (SEN-701)
feat(ui): Adds related issues to Incidents sidebar [SEN-695] (#13617)
...
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@markstory@billyvg@lynnagara
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

feat(events-v2) Add a event graph to the event modal - #13568

Merged
markstory merged 14 commits into
masterfrom
sen-716-modal-chart
Jun 11, 2019
Merged

feat(events-v2) Add a event graph to the event modal#13568
markstory merged 14 commits into
masterfrom
sen-716-modal-chart

Conversation

@markstory

@markstorymarkstory commented Jun 6, 2019

Copy link
Copy Markdown
Member

Show an event graph for grouped views. The grouped view mode requires showing a single event + grouped data above the single event view. I've implemented a stripped down line graph as we don't need release markers or previous period markers on this graph. Additionally we'll need special click handling that navigates to other events in the series.

TODO

  • Add UI integration jest tests

Refs SEN-716

@markstory
markstory requested a review from a teamJune 6, 2019 15:02

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How do you specify multiple groupIds?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Right now I don't have use-case where I need more than one group, I'll expand the proptype to allow for future scenarios though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The _ is not necessary here

@markstory

Copy link
Copy Markdown
MemberAuthor

Screen Shot 2019-06-06 at 1 50 28 PM

Screenshot of what it looks like in action

@markstory
markstoryforce-pushed the sen-716-modal-chart branch from f806d94 to cc390b3CompareJune 7, 2019 14:55
@markstory
markstory changed the base branch from sen-716-endpoint-last-event to feat/org-event-detailsJune 7, 2019 14:55
@markstory
markstory requested a review from lynnagaraJune 7, 2019 14:56
@markstory

Copy link
Copy Markdown
MemberAuthor

@lynnagara I've rebased this on top of your branch and switched to using the /latest endpoint when getting event information for grouped views.

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: PropTypes.string,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could have a proptype that enforces the eventSlug format

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sure!

Comment threadsrc/sentry/api/urls.py Outdated
@markstory
markstory changed the base branch from feat/org-event-details to masterJune 10, 2019 17:53
Show an event graph for grouped views. The grouped view mode requires
showing a single event + grouped data above the single event view. I've
implemented a stripped down line graph as we don't need release markers
or previous period markers on this graph. Additionally we'll need
special click handling that navigates to other events in the series.
Refs SEN-716
Use the new /latest endpoint in the grouped modal view. This pushes the
complexity of finding the most recent event into the API where it should
be.
Lyn prefers to keep the URL parameter `:` separated.
By creating a combined query parameter we have fewer state bits we need
to manage together. Instead we can focus on the valid/invalid value
states.
In grouped views we want to show pagination buttons to navigate between
the events that match the current search filters including the query
string. By creating URLs that use eventSlug and the current search
criteria we'll end up at the right events.
Refs SEN-715
@markstory
markstoryforce-pushed the sen-716-modal-chart branch from 7681f97 to 36f6b86CompareJune 10, 2019 19:54
@markstory

Copy link
Copy Markdown
MemberAuthor

I've included the pagination changes in this pull request now as well. The top half of the event modal looks like:

Screen Shot 2019-06-10 at 11 50 47 AM

@markstory
markstory requested review from a team and lynnagaraJune 11, 2019 14:44

const slugValidator = function(props, propName, componentName) {
const value = props[propName];
if (value && !/^(?:[^:]+:)?(?:[^:]+):(?:[a-f0-9]+|latest|oldest)$/.test(value)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:nice:regex:

It'd help to have a comment with what a valid slug looks like. Are special characters (non alphanumerics) allowed in the first two groups?

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: slugValidator,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to validate these at runtime as well? (I guess receiving a 404 makes sense)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think the 404 we display in the modal could use some work, but I can do that separately.

const ModalLineGraph = props => {
const {api, groupId, organization, location, selection} = props;

const isUtc = selection.datetime.utc;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So if selection.datetime.utc is null it won't necessarily mean that the user doesn't want it to be formatted UTC, it just means that they haven't use the UTC override in the global selection header. I think we'd want to fallback to user's timezone in ConfigStore.

Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
@markstory
markstory merged commit dc4fbea into masterJun 11, 2019
@markstory
markstory deleted the sen-716-modal-chart branch June 11, 2019 20:50
jan-auer added a commit that referenced this pull request Jun 12, 2019
* master: (102 commits)
fix(symbolicator): Tests are no longer allowed failures (#13566)
fix(yarn): Fix yarn path in distutils (#13647)
ref(ui): Remove OrganizationState from a bunch of places (#13646)
ref(ui): Remove crufty global exports (#13578)
feat(events-v2): Page loader improvements (#13645)
fix: Query the right set of issues when given no set of envs for a given first release (#13551)
feat(ui): update incidents icon (#13622)
fix(ui): remove rate-limited values from graph when no events are present (#13624)
feat(events-v2) Add a event graph to the event modal (#13568)
fix(store): Check project.status is VISIBLE before attempting to process
perf(snuba): Limit SnubaEvent.get_event to return the first result only. (#13644)
tests(percy): Fix Incident status item dynamic time (#13640)
ref(sentry10): Clean up action creators (#13636)
ref(app-platform): Backfill IntegrationFeature table (#13548)
ref(utils): Remove default / reexports from utils.jsx (#13581)
feat(sentry10): Remove the ability to set a default environment (#13637)
feat(api): Add url unfurls for incidents to slack integration (SEN-566)
build(yarn): Upgrade yarn to 1.16.0, embed in repo (#13569)
feat(api): Allow passing group_ids to `OrganizationGroupIndexEndpoint` for bulk fetching groups (SEN-701)
feat(ui): Adds related issues to Incidents sidebar [SEN-695] (#13617)
...
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@markstory@billyvg@lynnagara
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(events-v2) Add a event graph to the event modal - #13568

Merged
markstory merged 14 commits into
masterfrom
sen-716-modal-chart
Jun 11, 2019
Merged

feat(events-v2) Add a event graph to the event modal#13568
markstory merged 14 commits into
masterfrom
sen-716-modal-chart

Conversation

@markstory

@markstorymarkstory commented Jun 6, 2019

Copy link
Copy Markdown
Member

Show an event graph for grouped views. The grouped view mode requires showing a single event + grouped data above the single event view. I've implemented a stripped down line graph as we don't need release markers or previous period markers on this graph. Additionally we'll need special click handling that navigates to other events in the series.

TODO

  • Add UI integration jest tests

Refs SEN-716

@markstory
markstory requested a review from a teamJune 6, 2019 15:02

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How do you specify multiple groupIds?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Right now I don't have use-case where I need more than one group, I'll expand the proptype to allow for future scenarios though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The _ is not necessary here

@markstory

Copy link
Copy Markdown
MemberAuthor

Screen Shot 2019-06-06 at 1 50 28 PM

Screenshot of what it looks like in action

@markstory
markstoryforce-pushed the sen-716-modal-chart branch from f806d94 to cc390b3CompareJune 7, 2019 14:55
@markstory
markstory changed the base branch from sen-716-endpoint-last-event to feat/org-event-detailsJune 7, 2019 14:55
@markstory
markstory requested a review from lynnagaraJune 7, 2019 14:56
@markstory

Copy link
Copy Markdown
MemberAuthor

@lynnagara I've rebased this on top of your branch and switched to using the /latest endpoint when getting event information for grouped views.

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: PropTypes.string,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could have a proptype that enforces the eventSlug format

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sure!

Comment threadsrc/sentry/api/urls.py Outdated
@markstory
markstory changed the base branch from feat/org-event-details to masterJune 10, 2019 17:53
Show an event graph for grouped views. The grouped view mode requires
showing a single event + grouped data above the single event view. I've
implemented a stripped down line graph as we don't need release markers
or previous period markers on this graph. Additionally we'll need
special click handling that navigates to other events in the series.
Refs SEN-716
Use the new /latest endpoint in the grouped modal view. This pushes the
complexity of finding the most recent event into the API where it should
be.
Lyn prefers to keep the URL parameter `:` separated.
By creating a combined query parameter we have fewer state bits we need
to manage together. Instead we can focus on the valid/invalid value
states.
In grouped views we want to show pagination buttons to navigate between
the events that match the current search filters including the query
string. By creating URLs that use eventSlug and the current search
criteria we'll end up at the right events.
Refs SEN-715
@markstory
markstoryforce-pushed the sen-716-modal-chart branch from 7681f97 to 36f6b86CompareJune 10, 2019 19:54
@markstory

Copy link
Copy Markdown
MemberAuthor

I've included the pagination changes in this pull request now as well. The top half of the event modal looks like:

Screen Shot 2019-06-10 at 11 50 47 AM

@markstory
markstory requested review from a team and lynnagaraJune 11, 2019 14:44

const slugValidator = function(props, propName, componentName) {
const value = props[propName];
if (value && !/^(?:[^:]+:)?(?:[^:]+):(?:[a-f0-9]+|latest|oldest)$/.test(value)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:nice:regex:

It'd help to have a comment with what a valid slug looks like. Are special characters (non alphanumerics) allowed in the first two groups?

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: slugValidator,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to validate these at runtime as well? (I guess receiving a 404 makes sense)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think the 404 we display in the modal could use some work, but I can do that separately.

const ModalLineGraph = props => {
const {api, groupId, organization, location, selection} = props;

const isUtc = selection.datetime.utc;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So if selection.datetime.utc is null it won't necessarily mean that the user doesn't want it to be formatted UTC, it just means that they haven't use the UTC override in the global selection header. I think we'd want to fallback to user's timezone in ConfigStore.

Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
@markstory
markstory merged commit dc4fbea into masterJun 11, 2019
@markstory
markstory deleted the sen-716-modal-chart branch June 11, 2019 20:50
jan-auer added a commit that referenced this pull request Jun 12, 2019
* master: (102 commits)
fix(symbolicator): Tests are no longer allowed failures (#13566)
fix(yarn): Fix yarn path in distutils (#13647)
ref(ui): Remove OrganizationState from a bunch of places (#13646)
ref(ui): Remove crufty global exports (#13578)
feat(events-v2): Page loader improvements (#13645)
fix: Query the right set of issues when given no set of envs for a given first release (#13551)
feat(ui): update incidents icon (#13622)
fix(ui): remove rate-limited values from graph when no events are present (#13624)
feat(events-v2) Add a event graph to the event modal (#13568)
fix(store): Check project.status is VISIBLE before attempting to process
perf(snuba): Limit SnubaEvent.get_event to return the first result only. (#13644)
tests(percy): Fix Incident status item dynamic time (#13640)
ref(sentry10): Clean up action creators (#13636)
ref(app-platform): Backfill IntegrationFeature table (#13548)
ref(utils): Remove default / reexports from utils.jsx (#13581)
feat(sentry10): Remove the ability to set a default environment (#13637)
feat(api): Add url unfurls for incidents to slack integration (SEN-566)
build(yarn): Upgrade yarn to 1.16.0, embed in repo (#13569)
feat(api): Allow passing group_ids to `OrganizationGroupIndexEndpoint` for bulk fetching groups (SEN-701)
feat(ui): Adds related issues to Incidents sidebar [SEN-695] (#13617)
...
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@markstory@billyvg@lynnagara
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(events-v2) Add a event graph to the event modal - #13568

Merged
markstory merged 14 commits into
masterfrom
sen-716-modal-chart
Jun 11, 2019
Merged

feat(events-v2) Add a event graph to the event modal#13568
markstory merged 14 commits into
masterfrom
sen-716-modal-chart

Conversation

@markstory

@markstorymarkstory commented Jun 6, 2019

Copy link
Copy Markdown
Member

Show an event graph for grouped views. The grouped view mode requires showing a single event + grouped data above the single event view. I've implemented a stripped down line graph as we don't need release markers or previous period markers on this graph. Additionally we'll need special click handling that navigates to other events in the series.

TODO

  • Add UI integration jest tests

Refs SEN-716

@markstory
markstory requested a review from a teamJune 6, 2019 15:02

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How do you specify multiple groupIds?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Right now I don't have use-case where I need more than one group, I'll expand the proptype to allow for future scenarios though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The _ is not necessary here

@markstory

Copy link
Copy Markdown
MemberAuthor

Screen Shot 2019-06-06 at 1 50 28 PM

Screenshot of what it looks like in action

@markstory
markstoryforce-pushed the sen-716-modal-chart branch from f806d94 to cc390b3CompareJune 7, 2019 14:55
@markstory
markstory changed the base branch from sen-716-endpoint-last-event to feat/org-event-detailsJune 7, 2019 14:55
@markstory
markstory requested a review from lynnagaraJune 7, 2019 14:56
@markstory

Copy link
Copy Markdown
MemberAuthor

@lynnagara I've rebased this on top of your branch and switched to using the /latest endpoint when getting event information for grouped views.

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: PropTypes.string,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could have a proptype that enforces the eventSlug format

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sure!

Comment threadsrc/sentry/api/urls.py Outdated
@markstory
markstory changed the base branch from feat/org-event-details to masterJune 10, 2019 17:53
Show an event graph for grouped views. The grouped view mode requires
showing a single event + grouped data above the single event view. I've
implemented a stripped down line graph as we don't need release markers
or previous period markers on this graph. Additionally we'll need
special click handling that navigates to other events in the series.
Refs SEN-716
Use the new /latest endpoint in the grouped modal view. This pushes the
complexity of finding the most recent event into the API where it should
be.
Lyn prefers to keep the URL parameter `:` separated.
By creating a combined query parameter we have fewer state bits we need
to manage together. Instead we can focus on the valid/invalid value
states.
In grouped views we want to show pagination buttons to navigate between
the events that match the current search filters including the query
string. By creating URLs that use eventSlug and the current search
criteria we'll end up at the right events.
Refs SEN-715
@markstory
markstoryforce-pushed the sen-716-modal-chart branch from 7681f97 to 36f6b86CompareJune 10, 2019 19:54
@markstory

Copy link
Copy Markdown
MemberAuthor

I've included the pagination changes in this pull request now as well. The top half of the event modal looks like:

Screen Shot 2019-06-10 at 11 50 47 AM

@markstory
markstory requested review from a team and lynnagaraJune 11, 2019 14:44

const slugValidator = function(props, propName, componentName) {
const value = props[propName];
if (value && !/^(?:[^:]+:)?(?:[^:]+):(?:[a-f0-9]+|latest|oldest)$/.test(value)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:nice:regex:

It'd help to have a comment with what a valid slug looks like. Are special characters (non alphanumerics) allowed in the first two groups?

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: slugValidator,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to validate these at runtime as well? (I guess receiving a 404 makes sense)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think the 404 we display in the modal could use some work, but I can do that separately.

const ModalLineGraph = props => {
const {api, groupId, organization, location, selection} = props;

const isUtc = selection.datetime.utc;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So if selection.datetime.utc is null it won't necessarily mean that the user doesn't want it to be formatted UTC, it just means that they haven't use the UTC override in the global selection header. I think we'd want to fallback to user's timezone in ConfigStore.

Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
@markstory
markstory merged commit dc4fbea into masterJun 11, 2019
@markstory
markstory deleted the sen-716-modal-chart branch June 11, 2019 20:50
jan-auer added a commit that referenced this pull request Jun 12, 2019
* master: (102 commits)
fix(symbolicator): Tests are no longer allowed failures (#13566)
fix(yarn): Fix yarn path in distutils (#13647)
ref(ui): Remove OrganizationState from a bunch of places (#13646)
ref(ui): Remove crufty global exports (#13578)
feat(events-v2): Page loader improvements (#13645)
fix: Query the right set of issues when given no set of envs for a given first release (#13551)
feat(ui): update incidents icon (#13622)
fix(ui): remove rate-limited values from graph when no events are present (#13624)
feat(events-v2) Add a event graph to the event modal (#13568)
fix(store): Check project.status is VISIBLE before attempting to process
perf(snuba): Limit SnubaEvent.get_event to return the first result only. (#13644)
tests(percy): Fix Incident status item dynamic time (#13640)
ref(sentry10): Clean up action creators (#13636)
ref(app-platform): Backfill IntegrationFeature table (#13548)
ref(utils): Remove default / reexports from utils.jsx (#13581)
feat(sentry10): Remove the ability to set a default environment (#13637)
feat(api): Add url unfurls for incidents to slack integration (SEN-566)
build(yarn): Upgrade yarn to 1.16.0, embed in repo (#13569)
feat(api): Allow passing group_ids to `OrganizationGroupIndexEndpoint` for bulk fetching groups (SEN-701)
feat(ui): Adds related issues to Incidents sidebar [SEN-695] (#13617)
...
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@markstory@billyvg@lynnagara
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

feat(events-v2) Add a event graph to the event modal - #13568

Merged
markstory merged 14 commits into
masterfrom
sen-716-modal-chart
Jun 11, 2019
Merged

feat(events-v2) Add a event graph to the event modal#13568
markstory merged 14 commits into
masterfrom
sen-716-modal-chart

Conversation

@markstory

@markstorymarkstory commented Jun 6, 2019

Copy link
Copy Markdown
Member

Show an event graph for grouped views. The grouped view mode requires showing a single event + grouped data above the single event view. I've implemented a stripped down line graph as we don't need release markers or previous period markers on this graph. Additionally we'll need special click handling that navigates to other events in the series.

TODO

  • Add UI integration jest tests

Refs SEN-716

@markstory
markstory requested a review from a teamJune 6, 2019 15:02

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How do you specify multiple groupIds?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Right now I don't have use-case where I need more than one group, I'll expand the proptype to allow for future scenarios though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The _ is not necessary here

@markstory

Copy link
Copy Markdown
MemberAuthor

Screen Shot 2019-06-06 at 1 50 28 PM

Screenshot of what it looks like in action

@markstory
markstoryforce-pushed the sen-716-modal-chart branch from f806d94 to cc390b3CompareJune 7, 2019 14:55
@markstory
markstory changed the base branch from sen-716-endpoint-last-event to feat/org-event-detailsJune 7, 2019 14:55
@markstory
markstory requested a review from lynnagaraJune 7, 2019 14:56
@markstory

Copy link
Copy Markdown
MemberAuthor

@lynnagara I've rebased this on top of your branch and switched to using the /latest endpoint when getting event information for grouped views.

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: PropTypes.string,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could have a proptype that enforces the eventSlug format

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sure!

Comment threadsrc/sentry/api/urls.py Outdated
@markstory
markstory changed the base branch from feat/org-event-details to masterJune 10, 2019 17:53
Show an event graph for grouped views. The grouped view mode requires
showing a single event + grouped data above the single event view. I've
implemented a stripped down line graph as we don't need release markers
or previous period markers on this graph. Additionally we'll need
special click handling that navigates to other events in the series.
Refs SEN-716
Use the new /latest endpoint in the grouped modal view. This pushes the
complexity of finding the most recent event into the API where it should
be.
Lyn prefers to keep the URL parameter `:` separated.
By creating a combined query parameter we have fewer state bits we need
to manage together. Instead we can focus on the valid/invalid value
states.
In grouped views we want to show pagination buttons to navigate between
the events that match the current search filters including the query
string. By creating URLs that use eventSlug and the current search
criteria we'll end up at the right events.
Refs SEN-715
@markstory
markstoryforce-pushed the sen-716-modal-chart branch from 7681f97 to 36f6b86CompareJune 10, 2019 19:54
@markstory

Copy link
Copy Markdown
MemberAuthor

I've included the pagination changes in this pull request now as well. The top half of the event modal looks like:

Screen Shot 2019-06-10 at 11 50 47 AM

@markstory
markstory requested review from a team and lynnagaraJune 11, 2019 14:44

const slugValidator = function(props, propName, componentName) {
const value = props[propName];
if (value && !/^(?:[^:]+:)?(?:[^:]+):(?:[a-f0-9]+|latest|oldest)$/.test(value)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:nice:regex:

It'd help to have a comment with what a valid slug looks like. Are special characters (non alphanumerics) allowed in the first two groups?

params: PropTypes.object,
eventSlug: PropTypes.string.isRequired,
organization: SentryTypes.Organization.isRequired,
eventSlug: slugValidator,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to validate these at runtime as well? (I guess receiving a 404 makes sense)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think the 404 we display in the modal could use some work, but I can do that separately.

const ModalLineGraph = props => {
const {api, groupId, organization, location, selection} = props;

const isUtc = selection.datetime.utc;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So if selection.datetime.utc is null it won't necessarily mean that the user doesn't want it to be formatted UTC, it just means that they haven't use the UTC override in the global selection header. I think we'd want to fallback to user's timezone in ConfigStore.

Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
Comment threadsrc/sentry/static/sentry/app/views/organizationEventsV2/modalPagination.jsx Outdated
@markstory
markstory merged commit dc4fbea into masterJun 11, 2019
@markstory
markstory deleted the sen-716-modal-chart branch June 11, 2019 20:50
jan-auer added a commit that referenced this pull request Jun 12, 2019
* master: (102 commits)
fix(symbolicator): Tests are no longer allowed failures (#13566)
fix(yarn): Fix yarn path in distutils (#13647)
ref(ui): Remove OrganizationState from a bunch of places (#13646)
ref(ui): Remove crufty global exports (#13578)
feat(events-v2): Page loader improvements (#13645)
fix: Query the right set of issues when given no set of envs for a given first release (#13551)
feat(ui): update incidents icon (#13622)
fix(ui): remove rate-limited values from graph when no events are present (#13624)
feat(events-v2) Add a event graph to the event modal (#13568)
fix(store): Check project.status is VISIBLE before attempting to process
perf(snuba): Limit SnubaEvent.get_event to return the first result only. (#13644)
tests(percy): Fix Incident status item dynamic time (#13640)
ref(sentry10): Clean up action creators (#13636)
ref(app-platform): Backfill IntegrationFeature table (#13548)
ref(utils): Remove default / reexports from utils.jsx (#13581)
feat(sentry10): Remove the ability to set a default environment (#13637)
feat(api): Add url unfurls for incidents to slack integration (SEN-566)
build(yarn): Upgrade yarn to 1.16.0, embed in repo (#13569)
feat(api): Allow passing group_ids to `OrganizationGroupIndexEndpoint` for bulk fetching groups (SEN-701)
feat(ui): Adds related issues to Incidents sidebar [SEN-695] (#13617)
...
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 20, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@markstory@billyvg@lynnagara