Browse Source

feat(tags): tag facets update clickable expand area (#43567)

Updates the tag key, top value, and bar area to be clickable to expand
the tag distribution.
Also makes single value tags not clickable (since there's no point
here).
edwardgou-sentry 2 years ago
parent
commit
9a67a00535

+ 26 - 1
static/app/components/group/tagFacets/index.spec.tsx

@@ -1,5 +1,5 @@
 import {initializeOrg} from 'sentry-test/initializeOrg';
-import {render, screen, waitFor} from 'sentry-test/reactTestingLibrary';
+import {render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary';
 
 import TagFacets, {TAGS_FORMATTER} from 'sentry/components/group/tagFacets';
 
@@ -157,5 +157,30 @@ describe('Tag Facets', function () {
       expect(screen.getByText('iOS 16.0')).toBeInTheDocument();
       expect(screen.getAllByText('Android 12').length).toEqual(2);
     });
+
+    it('closes and expands tag distribution when tag header is clicked', async function () {
+      render(
+        <TagFacets
+          environments={[]}
+          groupId="1"
+          project={project}
+          tagKeys={tags}
+          tagFormatter={TAGS_FORMATTER}
+        />,
+        {
+          organization,
+        }
+      );
+      await waitFor(() => {
+        expect(tagsMock).toHaveBeenCalled();
+      });
+      expect(screen.getByText('iOS 16.0')).toBeInTheDocument();
+
+      userEvent.click(screen.getByText('os'));
+      expect(screen.queryByText('iOS 16.0')).not.toBeInTheDocument();
+
+      userEvent.click(screen.getByText('os'));
+      expect(screen.getByText('iOS 16.0')).toBeInTheDocument();
+    });
   });
 });

+ 34 - 33
static/app/components/group/tagFacets/tagFacetsDistributionMeter.tsx

@@ -5,7 +5,6 @@ import debounce from 'lodash/debounce';
 
 import {TagSegment} from 'sentry/actionCreators/events';
 import Link from 'sentry/components/links/link';
-import {SegmentValue} from 'sentry/components/tagDistributionMeter';
 import {IconChevron} from 'sentry/icons/iconChevron';
 import {t} from 'sentry/locale';
 import space from 'sentry/styles/space';
@@ -63,7 +62,8 @@ function TagFacetsDistributionMeter({
   expandByDefault,
 }: Props) {
   const organization = useOrganization();
-  const [expanded, setExpanded] = useState<boolean>(!!expandByDefault);
+  const multiValueTag = segments.length > 1;
+  const [expanded, setExpanded] = useState<boolean>(multiValueTag && !!expandByDefault);
   const [hoveredValue, setHoveredValue] = useState<TagSegment | null>(null);
   const debounceSetHovered = debounce(value => setHoveredValue(value), 70);
   const topSegments = segments.slice(0, MAX_SEGMENTS);
@@ -80,17 +80,14 @@ function TagFacetsDistributionMeter({
     return (
       <Title>
         <TitleType>{title}</TitleType>
-        <TitleDescription>
-          <Label>{topSegments[0].name || t('n/a')}</Label>
-        </TitleDescription>
-        <StyledChevron
-          direction={expanded ? 'up' : 'down'}
-          size="xs"
-          onClick={() => {
-            setExpanded(!expanded);
-          }}
-          aria-label={`expand-${title}`}
-        />
+        <TitleDescription>{topSegments[0].name || t('n/a')}</TitleDescription>
+        {multiValueTag && (
+          <StyledChevron
+            direction={expanded ? 'up' : 'down'}
+            size="xs"
+            aria-label={`expand-${title}`}
+          />
+        )}
       </Title>
     );
   }
@@ -109,9 +106,8 @@ function TagFacetsDistributionMeter({
         {topSegments.map((value, index) => {
           const pct = percent(value.count, totalValues);
           const pctLabel = Math.floor(pct);
-          const segmentProps: SegmentValue = {
+          const segmentProps = {
             index,
-            to: value.url,
             onClick: () => {
               trackAdvancedAnalyticsEvent('issue_group_details.tags.bar.clicked', {
                 tag: title,
@@ -142,15 +138,7 @@ function TagFacetsDistributionMeter({
               {value.isOther ? (
                 <OtherSegment aria-label={t('Other')} color={colors[colors.length - 1]} />
               ) : (
-                <Segment
-                  aria-label={t(
-                    'Add the %s %s segment tag to the search query',
-                    title,
-                    value.value
-                  )}
-                  color={colors[index]}
-                  {...segmentProps}
-                >
+                <Segment color={colors[index]} {...segmentProps}>
                   {/* if the first segment is 6% or less, the label won't fit cleanly into the segment, so don't show the label */}
                   {index === 0 && pctLabel > 6 ? `${pctLabel}%` : null}
                 </Segment>
@@ -170,7 +158,15 @@ function TagFacetsDistributionMeter({
           const unfocus = !!hoveredValue && hoveredValue.value !== segment.value;
           const focus = hoveredValue?.value === segment.value;
           return (
-            <Link key={`segment-${segment.name}-${index}`} to={segment.url}>
+            <Link
+              key={`segment-${segment.name}-${index}`}
+              to={segment.url}
+              aria-label={t(
+                'Add the %s %s segment tag to the search query',
+                title,
+                segment.value
+              )}
+            >
               <LegendRow
                 onMouseOver={() => debounceSetHovered(segment)}
                 onMouseLeave={() => debounceSetHovered(null)}
@@ -203,8 +199,13 @@ function TagFacetsDistributionMeter({
 
   return (
     <TagSummary>
-      {renderTitle()}
-      {renderSegments()}
+      <TagHeader
+        clickable={multiValueTag}
+        onClick={() => multiValueTag && setExpanded(!expanded)}
+      >
+        {renderTitle()}
+        {renderSegments()}
+      </TagHeader>
       {expanded && renderLegend()}
     </TagSummary>
   );
@@ -216,6 +217,10 @@ const TagSummary = styled('div')`
   margin-bottom: ${space(2)};
 `;
 
+const TagHeader = styled('span')<{clickable?: boolean}>`
+  ${p => (p.clickable ? 'cursor: pointer' : null)};
+`;
+
 const SegmentBar = styled('div')`
   display: flex;
   overflow: hidden;
@@ -239,6 +244,7 @@ const TitleType = styled('div')`
 `;
 
 const TitleDescription = styled('div')`
+  ${p => p.theme.overflowEllipsis};
   display: flex;
   color: ${p => p.theme.gray300};
   text-align: right;
@@ -246,10 +252,6 @@ const TitleDescription = styled('div')`
   ${p => p.theme.overflowEllipsis};
 `;
 
-const Label = styled('div')`
-  ${p => p.theme.overflowEllipsis};
-`;
-
 const OtherSegment = styled('span')<{color: string}>`
   display: block;
   width: 100%;
@@ -257,10 +259,9 @@ const OtherSegment = styled('span')<{color: string}>`
   color: inherit;
   outline: none;
   background-color: ${p => p.color};
-  cursor: pointer;
 `;
 
-const Segment = styled(Link, {shouldForwardProp: isPropValid})<{color: string}>`
+const Segment = styled('span', {shouldForwardProp: isPropValid})<{color: string}>`
   &:hover {
     color: ${p => p.theme.white};
   }

+ 15 - 3
static/app/views/discover/results.spec.tsx

@@ -130,11 +130,17 @@ function renderMockRequests() {
       },
       {
         key: 'environment',
-        topValues: [{count: 2, value: 'dev', name: 'dev'}],
+        topValues: [
+          {count: 2, value: 'dev', name: 'dev'},
+          {count: 1, value: 'prod', name: 'prod'},
+        ],
       },
       {
         key: 'foo',
-        topValues: [{count: 1, value: 'bar', name: 'bar'}],
+        topValues: [
+          {count: 2, value: 'bar', name: 'bar'},
+          {count: 1, value: 'baz', name: 'baz'},
+        ],
       },
     ],
   });
@@ -914,7 +920,7 @@ describe('Results', function () {
         },
       });
 
-      renderMockRequests();
+      const mockRequests = renderMockRequests();
 
       ProjectsStore.loadInitialData([TestStubs.Project()]);
 
@@ -934,6 +940,12 @@ describe('Results', function () {
 
       userEvent.click(await screen.findByRole('button', {name: 'Show Tags'}));
 
+      await waitFor(() => expect(mockRequests.eventFacetsMock).toHaveBeenCalled());
+
+      // TODO(edward): update this to be less generic
+      userEvent.click(screen.getByText('environment'));
+      userEvent.click(screen.getByText('foo'));
+
       // since environment collides with the environment field, it is wrapped with `tags[...]`
       expect(
         await screen.findByRole('link', {

+ 13 - 8
static/app/views/discover/tags.spec.jsx

@@ -22,15 +22,18 @@ describe('Tags', function () {
       body: [
         {
           key: 'release',
-          topValues: [{count: 2, value: '123abcd', name: '123abcd'}],
+          topValues: [{count: 3, value: '123abcd', name: '123abcd'}],
         },
         {
           key: 'environment',
-          topValues: [{count: 2, value: 'abcd123', name: 'abcd123'}],
+          topValues: [
+            {count: 2, value: 'abcd123', name: 'abcd123'},
+            {count: 1, value: 'anotherOne', name: 'anotherOne'},
+          ],
         },
         {
           key: 'color',
-          topValues: [{count: 2, value: 'red', name: 'red'}],
+          topValues: [{count: 3, value: 'red', name: 'red'}],
         },
       ],
     });
@@ -53,7 +56,7 @@ describe('Tags', function () {
       <Tags
         eventView={view}
         api={api}
-        totalValues={2}
+        totalValues={3}
         organization={org}
         selection={{projects: [], environments: [], datetime: {}}}
         location={{query: {}}}
@@ -92,7 +95,7 @@ describe('Tags', function () {
         eventView={view}
         api={api}
         organization={org}
-        totalValues={2}
+        totalValues={3}
         selection={{projects: [], environments: [], datetime: {}}}
         location={initialData.router.location}
         generateUrl={generateUrl}
@@ -105,6 +108,7 @@ describe('Tags', function () {
     await waitForElementToBeRemoved(
       () => screen.queryAllByTestId('loading-placeholder')[0]
     );
+    userEvent.click(screen.getByText('environment'));
 
     userEvent.click(
       screen.getByLabelText('Add the environment abcd123 segment tag to the search query')
@@ -126,7 +130,7 @@ describe('Tags', function () {
       <Tags
         eventView={view}
         api={api}
-        totalValues={2}
+        totalValues={3}
         organization={org}
         selection={{projects: [], environments: [], datetime: {}}}
         location={{query: {}}}
@@ -140,11 +144,12 @@ describe('Tags', function () {
     );
 
     expect(screen.getByText('release')).toBeInTheDocument();
-    expect(screen.getAllByText('123abcd').length).toEqual(2);
+    expect(screen.getByText('123abcd')).toBeInTheDocument();
     expect(screen.getByText('environment')).toBeInTheDocument();
     expect(screen.getByText('abcd123')).toBeInTheDocument();
     expect(screen.getByText('color')).toBeInTheDocument();
     expect(screen.getByText('red')).toBeInTheDocument();
-    expect(screen.getAllByText('100%').length).toEqual(4);
+    expect(screen.getAllByText('100%').length).toEqual(2);
+    expect(screen.getByText('66%')).toBeInTheDocument();
   });
 });

+ 19 - 3
static/app/views/performance/transactionSummary/transactionOverview/index.spec.tsx

@@ -317,15 +317,24 @@ describe('Performance > TransactionSummary', function () {
         },
         {
           key: 'environment',
-          topValues: [{count: 2, value: 'dev', name: 'dev'}],
+          topValues: [
+            {count: 2, value: 'dev', name: 'dev'},
+            {count: 1, value: 'prod', name: 'prod'},
+          ],
         },
         {
           key: 'foo',
-          topValues: [{count: 1, value: 'bar', name: 'bar'}],
+          topValues: [
+            {count: 2, value: 'bar', name: 'bar'},
+            {count: 1, value: 'baz', name: 'baz'},
+          ],
         },
         {
           key: 'user',
-          topValues: [{count: 1, value: 'id:100', name: '100'}],
+          topValues: [
+            {count: 2, value: 'id:100', name: '100'},
+            {count: 1, value: 'id:101', name: '101'},
+          ],
         },
       ],
     });
@@ -784,6 +793,11 @@ describe('Performance > TransactionSummary', function () {
 
       await screen.findByText('Tag Summary');
 
+      userEvent.click(screen.getByText('environment'));
+      userEvent.click(screen.getByText('foo'));
+      // TODO(edward): Update to something better like getByLabel once we add better accessibility.
+      userEvent.click(screen.getAllByText('user')[1]);
+
       userEvent.click(
         screen.getByLabelText('Add the environment dev segment tag to the search query')
       );
@@ -1190,6 +1204,8 @@ describe('Performance > TransactionSummary', function () {
       });
 
       await screen.findByText('Tag Summary');
+      userEvent.click(screen.getByText('environment'));
+      userEvent.click(screen.getByText('foo'));
 
       userEvent.click(
         screen.getByLabelText('Add the environment dev segment tag to the search query')