Browse Source

ref(alert): Only add onClick listeners to top message portion (#33356)

* ref(alert): Only add onClick listeners to top message portion

* fix(tests): Fix failed tests

* ref(feature-disabled): Remove e.stopPropagation

This is no longer necessary given 7498a7dcf7537c560af0082e784c934a5b15c6bf.

* ref(feature-disabled): Remove e.stopPropagation

* ref(feature-disabled): Remove e.stopPropagation()ref(event-errors):

No longer necessary given 7498a7dcf7537c560af0082e784c934a5b15c6bf.

* fix(alerts): Remove redundant custom paddings

* fix: Remove space import in alertMessage

* ref(alert): Use direction prop on IconChevron

* ref(alert): Use condensed padding syntax
Vu Luong 2 years ago
parent
commit
21d03ac91d

+ 3 - 26
static/app/components/acl/featureDisabled.tsx

@@ -77,25 +77,11 @@ function FeatureDisabled({
           )}
         </HelpText>
         <Clipboard hideUnsupported value={installText(features, featureName)}>
-          <CopyButton
-            borderless
-            size="xsmall"
-            onClick={e => {
-              e.stopPropagation();
-              e.preventDefault();
-            }}
-            icon={<IconCopy size="xs" />}
-          >
+          <CopyButton borderless size="xsmall" icon={<IconCopy size="xs" />}>
             {t('Copy to Clipboard')}
           </CopyButton>
         </Clipboard>
-        <Pre
-          onClick={e => {
-            e.stopPropagation();
-            e.preventDefault();
-            selectText(e.target as HTMLElement);
-          }}
-        >
+        <Pre onClick={e => selectText(e.target as HTMLElement)}>
           <code>{installText(features, featureName)}</code>
         </Pre>
       </Fragment>
@@ -119,16 +105,7 @@ function FeatureDisabled({
             </ToggleButton>
           )}
         </FeatureDisabledMessage>
-        {showDescription && (
-          <HelpDescription
-            onClick={e => {
-              e.stopPropagation();
-              e.preventDefault();
-            }}
-          >
-            {renderHelp()}
-          </HelpDescription>
-        )}
+        {showDescription && <HelpDescription>{renderHelp()}</HelpDescription>}
       </Fragment>
     );
   }

+ 149 - 105
static/app/components/alert.tsx

@@ -1,6 +1,7 @@
 import {useState} from 'react';
 import {css} from '@emotion/react';
 import styled from '@emotion/styled';
+import {useHover} from '@react-aria/interactions';
 import classNames from 'classnames';
 
 import {IconCheckmark, IconChevron, IconInfo, IconNot, IconWarning} from 'sentry/icons';
@@ -19,41 +20,108 @@ export interface AlertProps extends React.HTMLAttributes<HTMLDivElement> {
 
 const DEFAULT_TYPE = 'info';
 
-const IconWrapper = styled('div')`
-  display: flex;
-  height: calc(${p => p.theme.fontSizeMedium} * ${p => p.theme.text.lineHeightBody});
-  margin-right: ${space(1)};
-  align-items: center;
-`;
+function Alert({
+  type = DEFAULT_TYPE,
+  showIcon = false,
+  opaque,
+  system,
+  expand,
+  trailingItems,
+  className,
+  children,
+  ...props
+}: AlertProps) {
+  const [isExpanded, setIsExpanded] = useState(false);
+  const showExpand = defined(expand);
+  const showTrailingItems = defined(trailingItems);
+
+  // Show the hover state (with darker borders) only when hovering over the
+  // IconWrapper or MessageContainer.
+  const {hoverProps: iconHoverProps, isHovered: iconIsHovered} = useHover({
+    isDisabled: !showExpand,
+  });
+  const {hoverProps: messageHoverProps, isHovered: messageIsHovered} = useHover({
+    isDisabled: !showExpand,
+  });
+
+  function getIcon() {
+    switch (type) {
+      case 'warning':
+        return <IconWarning />;
+      case 'success':
+        return <IconCheckmark />;
+      case 'error':
+        return <IconNot />;
+      case 'info':
+      default:
+        return <IconInfo />;
+    }
+  }
 
-const ContentWrapper = styled('div')`
-  width: 100%;
-`;
+  function handleClick() {
+    showExpand && setIsExpanded(!isExpanded);
+  }
 
-const TrailingItems = styled('div')`
-  display: grid;
-  grid-auto-flow: column;
-  grid-template-rows: 100%;
-  align-items: center;
-  gap: ${space(1)};
-  height: calc(${p => p.theme.fontSizeMedium} * ${p => p.theme.text.lineHeightBody});
-  margin-left: ${space(1)};
-`;
+  return (
+    <Wrap
+      type={type}
+      system={system}
+      opaque={opaque}
+      expand={expand}
+      hovered={iconIsHovered || messageIsHovered}
+      className={classNames(type ? `ref-${type}` : '', className)}
+      {...props}
+    >
+      {showIcon && (
+        <IconWrapper onClick={handleClick} {...iconHoverProps}>
+          {getIcon()}
+        </IconWrapper>
+      )}
+      <ContentWrapper>
+        <MessageContainer
+          onClick={handleClick}
+          showIcon={showIcon}
+          showTrailingItems={showTrailingItems}
+          {...messageHoverProps}
+        >
+          <Message>{children}</Message>
+          {(showExpand || showTrailingItems) && (
+            <TrailingItemsWrap>
+              <TrailingItems onClick={e => e.stopPropagation()}>
+                {trailingItems}
+              </TrailingItems>
+              {showExpand && (
+                <ExpandIconWrap>
+                  <IconChevron direction={isExpanded ? 'up' : 'down'} />
+                </ExpandIconWrap>
+              )}
+            </TrailingItemsWrap>
+          )}
+        </MessageContainer>
+        {isExpanded && (
+          <ExpandContainer>
+            {Array.isArray(expand) ? expand.map(item => item) : expand}
+          </ExpandContainer>
+        )}
+      </ContentWrapper>
+    </Wrap>
+  );
+}
 
 const alertStyles = ({
-  theme,
   type = DEFAULT_TYPE,
-  trailingItems,
   system,
   opaque,
   expand,
-}: AlertProps & {theme: Theme}) => {
-  const alertColors = theme.alert[type] ?? theme.alert[DEFAULT_TYPE];
+  hovered,
+  theme,
+}: AlertProps & {theme: Theme; hovered?: boolean}) => {
+  const alertColors = theme.alert[type];
+  const showExpand = defined(expand);
 
   return css`
     display: flex;
     margin: 0 0 ${space(2)};
-    padding: ${space(1.5)} ${space(2)};
     font-size: ${theme.fontSizeMedium};
     border-radius: ${theme.borderRadius};
     border: 1px solid ${alertColors.border};
@@ -61,8 +129,6 @@ const alertStyles = ({
       ? `linear-gradient(${alertColors.backgroundLight}, ${alertColors.backgroundLight}), linear-gradient(${theme.background}, ${theme.background})`
       : `${alertColors.backgroundLight}`};
 
-    ${defined(trailingItems) && `padding-right: ${space(1.5)};`}
-
     a:not([role='button']) {
       color: ${theme.textColor};
       text-decoration-color: ${theme.translucentBorder};
@@ -81,21 +147,27 @@ const alertStyles = ({
       margin: ${space(0.5)} 0 0;
     }
 
-    ${IconWrapper} {
+    ${IconWrapper}, ${ExpandIconWrap} {
       color: ${alertColors.iconColor};
     }
 
-    ${expand &&
+    ${hovered &&
     `
-      cursor: pointer;
-      &:hover {
-        border-color: ${alertColors.borderHover}
-      }
-      &:hover ${IconWrapper} {
+      border-color: ${alertColors.borderHover};
+      ${IconWrapper}, ${IconChevron} {
         color: ${alertColors.iconHoverColor};
       }
     `}
 
+    ${showExpand &&
+    `${IconWrapper}, ${MessageContainer} {
+        cursor: pointer;
+      }
+      ${TrailingItems} {
+       cursor: auto;
+      }
+    `}
+
     ${system &&
     `
       border-width: 0 0 1px 0;
@@ -104,92 +176,64 @@ const alertStyles = ({
   `;
 };
 
-const StyledTextBlock = styled('span')`
-  line-height: ${p => p.theme.text.lineHeightBody};
-  position: relative;
-  flex: 1;
+const Wrap = styled('div')<AlertProps & {hovered: boolean}>`
+  ${alertStyles}
 `;
 
-const MessageContainer = styled('div')`
+const IconWrapper = styled('div')`
   display: flex;
+  height: calc(${p => p.theme.fontSizeMedium} * ${p => p.theme.text.lineHeightBody});
+  padding: ${space(1.5)} ${space(1)} ${space(1.5)} ${space(2)};
+  box-sizing: content-box;
+  align-items: center;
+`;
+
+const ContentWrapper = styled('div')`
   width: 100%;
 `;
 
-const ExpandContainer = styled('div')`
+const MessageContainer = styled('div')<{
+  showIcon: boolean;
+  showTrailingItems: boolean;
+}>`
+  display: flex;
+  width: 100%;
+  padding-top: ${space(1.5)};
+  padding-bottom: ${space(1.5)};
+  padding-left: ${p => (p.showIcon ? '0' : space(2))};
+  padding-right: ${p => (p.showTrailingItems ? space(1.5) : space(2))};
+`;
+
+const Message = styled('span')`
+  line-height: ${p => p.theme.text.lineHeightBody};
+  position: relative;
+  flex: 1;
+`;
+
+const TrailingItems = styled('div')`
+  height: calc(${p => p.theme.fontSizeMedium} * ${p => p.theme.text.lineHeightBody});
   display: grid;
-  padding-top: ${space(1)};
+  grid-auto-flow: column;
+  grid-template-rows: 100%;
+  align-items: center;
+  gap: ${space(1)};
 `;
 
-const ExpandIcon = styled(props => (
-  <IconWrapper {...props}>{<IconChevron />}</IconWrapper>
-))`
-  transform: ${props => (props.isExpanded ? 'rotate(0deg)' : 'rotate(180deg)')};
-  justify-self: flex-end;
+const TrailingItemsWrap = styled(TrailingItems)`
+  margin-left: ${space(1)};
 `;
 
-const Alert = styled(
-  ({
-    type,
-    children,
-    className,
-    showIcon = false,
-    expand,
-    trailingItems,
-    opaque: _opaque, // don't forward to `div`
-    system: _system, // don't forward to `div`
-    ...props
-  }: AlertProps) => {
-    const [isExpanded, setIsExpanded] = useState(false);
-    const showExpand = defined(expand);
-    const showExpandItems = showExpand && isExpanded;
-
-    const getIcon = () => {
-      switch (type) {
-        case 'warning':
-          return <IconWarning />;
-        case 'success':
-          return <IconCheckmark />;
-        case 'error':
-          return <IconNot />;
-        case 'info':
-        default:
-          return <IconInfo />;
-      }
-    };
-
-    return (
-      <div
-        onClick={() => showExpand && setIsExpanded(!isExpanded)}
-        className={classNames(type ? `ref-${type}` : '', className)}
-        {...props}
-      >
-        {showIcon && <IconWrapper>{getIcon()}</IconWrapper>}
-        <ContentWrapper>
-          <MessageContainer>
-            <StyledTextBlock>{children}</StyledTextBlock>
-            {(showExpand || defined(trailingItems)) && (
-              <TrailingItems>
-                {trailingItems}
-                {showExpand && <ExpandIcon isExpanded={isExpanded} />}
-              </TrailingItems>
-            )}
-          </MessageContainer>
-          {showExpandItems && (
-            <ExpandContainer>
-              {Array.isArray(expand) ? expand.map(item => item) : expand}
-            </ExpandContainer>
-          )}
-        </ContentWrapper>
-      </div>
-    );
-  }
-)<AlertProps>`
-  ${alertStyles}
+const ExpandIconWrap = styled('div')`
+  height: 100%;
+  display: flex;
+  align-items: center;
 `;
 
-Alert.defaultProps = {
-  type: DEFAULT_TYPE,
-};
+const ExpandContainer = styled('div')`
+  display: grid;
+  padding-right: ${space(1.5)};
+  padding-bottom: ${space(1.5)};
+`;
 
 export {alertStyles};
 

+ 1 - 3
static/app/components/events/errorItem.tsx

@@ -50,9 +50,7 @@ class ErrorItem extends React.Component<Props, State> {
     return this.state.isOpen !== nextState.isOpen;
   }
 
-  handleToggle = (e: React.MouseEvent) => {
-    e.preventDefault();
-    e.stopPropagation();
+  handleToggle = () => {
     this.setState({isOpen: !this.state.isOpen});
   };
 

+ 5 - 7
static/app/components/events/errors.tsx

@@ -171,13 +171,11 @@ class Errors extends Component<Props, State> {
             </ErrorList>,
           ]}
         >
-          <span data-test-id="alert-summary-info">
-            {tn(
-              'There was %s problem processing this event',
-              'There were %s problems processing this event',
-              errors.length
-            )}
-          </span>
+          {tn(
+            'There was %s problem processing this event',
+            'There were %s problems processing this event',
+            errors.length
+          )}
         </StyledAlert>
       </StyledDataSection>
     );

+ 0 - 2
static/app/components/panels/panelAlert.tsx

@@ -2,14 +2,12 @@ import * as React from 'react';
 import styled from '@emotion/styled';
 
 import Alert from 'sentry/components/alert';
-import space from 'sentry/styles/space';
 
 type Props = React.ComponentProps<typeof Alert>;
 
 // Margin bottom should probably be a different prop
 const PanelAlert = styled(({...props}: Props) => <Alert {...props} showIcon system />)`
   margin: 0 0 1px 0;
-  padding: ${space(2)};
   border-radius: 0;
   box-shadow: none;
 

+ 0 - 2
static/app/views/app/alertMessage.tsx

@@ -6,7 +6,6 @@ import ExternalLink from 'sentry/components/links/externalLink';
 import {IconClose} from 'sentry/icons';
 import {t} from 'sentry/locale';
 import AlertStore from 'sentry/stores/alertStore';
-import space from 'sentry/styles/space';
 
 type Props = {
   alert: ReturnType<typeof AlertStore['getState']>[number];
@@ -42,7 +41,6 @@ const AlertMessage = ({alert, system}: Props) => {
 export default AlertMessage;
 
 const StyledAlert = styled(Alert)`
-  padding: ${space(1)} ${space(2)};
   margin: 0;
 `;
 

+ 4 - 4
tests/js/spec/components/stream/processingIssueHint.spec.jsx

@@ -41,7 +41,7 @@ describe('ProcessingIssueHint', function () {
     });
 
     it('displays text', function () {
-      const text = wrapper.find('StyledTextBlock').text();
+      const text = wrapper.find('Message').text();
       expect(text).toEqual(expect.stringContaining('issues blocking'));
     });
   });
@@ -65,7 +65,7 @@ describe('ProcessingIssueHint', function () {
     });
 
     it('displays text', function () {
-      const text = wrapper.find('StyledTextBlock').text();
+      const text = wrapper.find('Message').text();
       expect(text).toEqual(expect.stringContaining('Reprocessing'));
     });
   });
@@ -92,7 +92,7 @@ describe('ProcessingIssueHint', function () {
     });
 
     it('displays text', function () {
-      const text = wrapper.find('StyledTextBlock').text();
+      const text = wrapper.find('Message').text();
       expect(text).toEqual(expect.stringContaining('pending reprocessing'));
     });
   });
@@ -110,7 +110,7 @@ describe('ProcessingIssueHint', function () {
       );
     });
     it('displays the project slug', function () {
-      const text = wrapper.find('StyledTextBlock').text();
+      const text = wrapper.find('Message').text();
       expect(text).toEqual(expect.stringContaining(projectId));
     });
   });

+ 2 - 2
tests/js/spec/views/organizationGroupDetails/groupEventEntries.spec.tsx

@@ -31,9 +31,9 @@ async function renderComponent(event: Event, errors?: Array<Error>) {
   const eventErrors = wrapper.find('Errors');
 
   const alert = eventErrors.find('StyledAlert');
-  const alertSummaryInfo = alert.find('span[data-test-id="alert-summary-info"]');
+  const alertSummaryInfo = alert.find('MessageContainer');
 
-  alert.simulate('click');
+  alertSummaryInfo.simulate('click');
 
   await tick();
   wrapper.update();