Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(ts): Convert <SidebarOrgSummary> to typescript #16358

Merged
merged 52 commits into from
Jan 13, 2020

Conversation

matejminar
Copy link
Member

No description provided.

@matejminar matejminar requested a review from a team January 9, 2020 15:18
The implementation of this function from django 1.9.13 covers more invalid URLs
than the one from django-sudo does. We should not redirect to
`http:123347` style URLs.
When updating Gravatar to typescript, I removed some property spreads as
they triggered typescript warnings and the propTypes didn't cover any
props that needed spreading. The onLoad and onError props were load
bearing as they are used to handle gravatar fallbacks.
Copy link
Member

@dashed dashed left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

}
if (projects) {
// should we think about pluralization here?
extra.push(`${projects} projects`);
Copy link
Member

Choose a reason for hiding this comment

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

We likely should use tn() in the locale module to translation and pluralization.

Copy link
Member Author

Choose a reason for hiding this comment

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

used it, thanks 👍

wmak and others added 20 commits January 9, 2020 12:35
- Shows a permission error if a user without access to events like a
  billing account tries to access discover
…text

Have the trace_id and span_id use the built in columns instead of the context
array.
This removes `LatestContext` dependency and fixes `ContextPickerModal` and refactors it to ts.

Co-Authored-By: Matej Minar <[email protected]>
Also makes sure that the index redirects to combined alert listing view.
* Fetch tag values individually. This will let snuba use the promoted
  columns more effectively as they perform better.
* Only get 10 facets by default.
* Use a having clause instead of WHERE to exclude tracing tags as it
  performs better.
* Don't use fields that unpack into arrayJoin() expressions where
  possible.
* Enable sampling at lower project thresholds, as we don't need full
  accuracy.
Remove SnubaEvent since we are moving towards a single event model
(eventstore/models/Event) everywhere.
If an event hasn't been propagated when the user report is created, then queue a
task to sync the event data once it's stored fully.
The route for sentry-organization-auth-settings was still located in the
old settings path, this route is used for a redirect in the auth.helper
module when there is a problem configuring SSO.

This just corrects the redirect so it goes back to settings instead of
404ing.
The `sentry:user` tag can come back null when it is read from the
promoted columns. Handling nulls allows get_tag_value_label() to be used
more broadly.
Remove the automatic dataset detection logic from sentry. We have
switched over to the snuba based implementation of this. Not having
dataset selection in sentry also means we can remove some (but not all)
of the supporting logic as well.
This disables all form fields when editing a Metric Alert without the `org:write` role.

return (
<OrgSummary>
<OrganizationAvatar css={{flexShrink: 0}} organization={organization} size={36} />
Copy link
Member

Choose a reason for hiding this comment

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

css is a legitimate prop here via emotion

Copy link
Member Author

@matejminar matejminar Jan 10, 2020

Choose a reason for hiding this comment

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

That's what I thought, but after inspection, I found out that the css prop was not being applied here at all, that's why I changed it to style, which is according to underlying components' types being passed down and applied to the final element. But anyway, the final element (StyledBaseAvatar in app/components/avatar/baseAvatar) has flex-shrink: 0 by default, so do you think we can keep it removed altogether?

Copy link
Member

Choose a reason for hiding this comment

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

It does look like it's being applied, this is from production (I believe it's this component):
image

Copy link
Member Author

@matejminar matejminar Jan 10, 2020

Choose a reason for hiding this comment

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

Yes, it should be that component, but it's flex-shrink comes imho from:

const StyledBaseAvatar = styled('span')<{round: boolean; loaded: boolean}>`
flex-shrink: 0;
${p => !p.loaded && 'background-color: rgba(200, 200, 200, 0.1);'};
${p => p.round && 'border-radius: 100%;'};
`;

Copy link
Member

Choose a reason for hiding this comment

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

Ah, cool, let's kill it then

Copy link
Member Author

Choose a reason for hiding this comment

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

awesome, thank you

@matejminar matejminar requested a review from billyvg January 10, 2020 10:28
matejminar and others added 7 commits January 10, 2020 13:17
The facets endpoint is still not fast. I want to see what kind of
results can be obtained if we aggressively sample tag values. If this
yields acceptable performance I want to try scaling the sampling rate
based on the data volume so that we can better balance accurracy and
performance.
…6291)

* fix(discover2): Test fix on Percy snapshots
* fix(discover2): Improve QueryBuilder for error/loading states
* fix(discover2): Resize QueryBuilder when browser is resized
This removes the UI components that fetch and render related issues on the Incidents details view.
Move the implementation into the Pagerduty plugin since this is the only
place it is used.
markstory and others added 14 commits January 10, 2020 14:39
Add spans to collect more information about snuba queries. Having the
referrer and the start of the query body should make tracking down slow
queries in snuba easier as we have more context.
This removes the UI and endpoint to create a manual incident.
This was used on the Incidents details view, but is no longer used
Outputting the raw data when we also output all the formatted keys
wastes space and adds visual noise.
* ref(ui): Refactor `cx` from emotion to `classnames`

We generally use the `classnames` library to handle merging/dynamic `className`. No reason to use two different libraries.

* upgrade emotion, add styled wrapper for typings (and related paths), add eslint rules for emotion, update babel plugin

* Refactor usage of emotion `css` being treated as a class name string

* change innerRef --> ref

* refactor grid-emotion props and update to `reflexbox`

* eslint for emotion-setup

* refactor injectGlobal --> Global for emotion upgrade

* Add <CacheProvider>, this is needed to turn speedy off for percy

* refactor <HeaderItem> - fix missing props to styled components, remove innerRef and forwardRef

* Change <Tooltip> logic for when to apply a wrapper, emotion causes the previously expected types to be different

* random TS fixes for styled components

* fix <RadioGroup> animation needing `css`

* forwardRef for Switch

* define a `theme.space` for rebass/grid

* fix DOM attribute warnings with "loading" prop

* fix settings breadcrumb - Box in `reflexbox` has different CSS than grid-emotion

* change <ProviderRow> to remove `Flex` around CircleIndicator as it was squishing it on small widths

* css fixes for percy

* fix featureDisabled logic for custom alerts because emotion

* refactor emotion9 --> emotion10 (imports)

* update snapshots and fix tests
Fixes links to screenshots in README.md broken by 983c8c6
* ref(ts): Convert <SearchBar> to typescript

* removed proptypes, placeholder optional
@matejminar matejminar merged commit 7d72067 into master Jan 13, 2020
@matejminar matejminar deleted the ref/ts/sidebarOrgSummary branch January 13, 2020 10:21
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2020
Sign up for free to 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.