Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

feat(issue-views): Change add view button to just + icon near starred views header' #87360

Merged
merged 4 commits into from
Mar 19, 2025
Merged

feat(issue-views): Change add view button to just + icon near starred views header' #87360

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c58679b
Change add view button to just + icon near starred views header'
MichaelSun48 Mar 18, 2025
c13f91b
Remove negative margin
MichaelSun48 Mar 18, 2025
0a2c4c5
Update static/app/components/nav/issueViews/issueViewAddViewButton.tsx
MichaelSun48 Mar 19, 2025
b0708de
Add comment about style hack
MichaelSun48 Mar 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 27 additions & 58 deletions static/app/components/nav/issueViews/issueViewAddViewButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
import {Fragment, useState} from 'react';
import {css} from '@emotion/react';
import {useState} from 'react';
import styled from '@emotion/styled';
import {motion} from 'framer-motion';

import {Button} from 'sentry/components/core/button';
import LoadingIndicator from 'sentry/components/loadingIndicator';
import {useNavContext} from 'sentry/components/nav/context';
import useDefaultProject from 'sentry/components/nav/issueViews/useDefaultProject';
import {NavLayout} from 'sentry/components/nav/types';
import type {NavLayout} from 'sentry/components/nav/types';
import {IconAdd} from 'sentry/icons';
import {t} from 'sentry/locale';
import {space} from 'sentry/styles/space';
Expand Down Expand Up @@ -70,64 +68,35 @@ export function IssueViewAddViewButton({baseUrl}: {baseUrl: string}) {
};

return (
<motion.div>
<AddViewButton
borderless
size="zero"
layout={layout}
onClick={handleOnAddView}
disabled={isLoading}
>
{isLoading ? (
<LoadingIndicator mini />
<AddViewButton
borderless
size="zero"
layout={layout}
onClick={handleOnAddView}
disabled={isLoading}
title={!isLoading && t('Add View')}
aria-label={t('Add View Button')}
tooltipProps={{
delay: 500,
}}
icon={
isLoading ? (
<StyledLoadingIndicator mini size={14} />
) : (
<Fragment>
<StyledIconAdd size="xs" />
{t('Add View')}
</Fragment>
)}
</AddViewButton>
</motion.div>
<IconAdd size="sm" color="subText" />
)
}
/>
);
}

const AddViewButton = styled(Button)<{layout: NavLayout}>`
display: flex;
align-items: center;
justify-content: center;
padding: ${space(0.5)};

width: 100%;
position: relative;
height: 34px;
color: ${p => p.theme.textColor};
font-size: ${p => p.theme.fontSizeMedium};
font-weight: ${p => p.theme.fontWeightNormal};
line-height: 177.75%;
border-radius: ${p => p.theme.borderRadius};

&:hover {
color: inherit;
}

[data-isl] {
transform: translate(0, 0);
top: 1px;
bottom: 1px;
right: 0;
left: 0;
width: initial;
height: initial;
}

${p =>
p.layout === NavLayout.MOBILE &&
css`
padding: 0 ${space(1.5)} 0 48px;
border-radius: 0;
`}
const StyledLoadingIndicator = styled(LoadingIndicator)`
width: 14px;
height: 14px !important;
margin: 0 !important;
`;
Comment on lines +95 to +96
Copy link
Member Author

@MichaelSun48 MichaelSun48 Mar 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Idk why but there are top level sentry.css styles that are forcing margin and height properties on the loadingindicator.mini class.

image

Copy link
Member

Choose a reason for hiding this comment

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

Ugh I hate when we run into high specificity CSS styles

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if adding mini is conflicting with the height prop you're sending?

Copy link
Member Author

Choose a reason for hiding this comment

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

It might, but mini also controls the width of the loading ring. Without it here, the width looks too big relative to the space its contained in.

Copy link
Member

Choose a reason for hiding this comment

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

Ah that makes sense


const StyledIconAdd = styled(IconAdd)`
margin-right: 4px;
const AddViewButton = styled(Button)<{layout: NavLayout}>`
padding: ${space(0.5)};
margin-right: -${space(0.5)};
`;
9 changes: 6 additions & 3 deletions static/app/components/nav/issueViews/issueViewNavItemContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,8 @@ export function IssueViewNavItemContent({

const {startInteraction, endInteraction, isInteractingRef} = useNavContext();

const scrollPosition = window.scrollY || document.documentElement.scrollTop;

return (
<StyledReorderItem
as="div"
Expand All @@ -149,12 +151,13 @@ export function IssueViewNavItemContent({
dragListener={false}
dragControls={controls}
style={{
...(isDragging
...(isDragging || scrollPosition === 0
Copy link
Member Author

Choose a reason for hiding this comment

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

This kinda fixes that really annoying issue where views wouldn't animate in and out as a result of the originY styles. We only need to apply those styles if the user has scrolled down on the page, since that's what triggers the framer motion bug.

Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to add a comment here about why this is necessary since it will be very confusing to future readers!

? {}
: {
originY: '0px',
}),
}}
grabbing={isDragging === view.id}
>
<StyledSecondaryNavItem
to={constructViewLink(baseUrl, view)}
Expand Down Expand Up @@ -345,9 +348,9 @@ const hasUnsavedChanges = (

// Reorder.Item does handle lifting an item being dragged above other items out of the box,
// but we need to ensure the item is relatively positioned and has a background color for it to work
const StyledReorderItem = styled(Reorder.Item)`
const StyledReorderItem = styled(Reorder.Item)<{grabbing: boolean}>`
position: relative;
background-color: ${p => p.theme.translucentSurface200};
background-color: ${p => (p.grabbing ? p.theme.translucentSurface200 : 'transparent')};
Copy link
Member Author

Choose a reason for hiding this comment

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

Views had a slightly different background in their resting state. This applies the translucent border only when dragging.
image

border-radius: ${p => p.theme.borderRadius};
`;

Expand Down
44 changes: 30 additions & 14 deletions static/app/components/nav/issueViews/issueViewNavItems.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import {useCallback, useEffect, useMemo, useState} from 'react';
import {AnimatePresence, Reorder} from 'framer-motion';
import styled from '@emotion/styled';
import {Reorder} from 'framer-motion';
import debounce from 'lodash/debounce';
import isEqual from 'lodash/isEqual';

import {IssueViewAddViewButton} from 'sentry/components/nav/issueViews/issueViewAddViewButton';
import {IssueViewNavItemContent} from 'sentry/components/nav/issueViews/issueViewNavItemContent';
import {SecondaryNav} from 'sentry/components/nav/secondary';
import {normalizeDateTimeParams} from 'sentry/components/organizations/pageFilters/parse';
import {t} from 'sentry/locale';
import {trackAnalytics} from 'sentry/utils/analytics';
import {setApiQueryData, useQueryClient} from 'sentry/utils/queryClient';
import normalizeUrl from 'sentry/utils/url/normalizeUrl';
Expand Down Expand Up @@ -256,17 +259,25 @@ export function IssueViewNavItems({
);

return (
<Reorder.Group
as="div"
axis="y"
values={views}
onReorder={newOrder => setViews(newOrder)}
initial={false}
ref={sectionRef}
<SecondaryNav.Section
title={
<TitleWrapper>
{t('Starred Views')}
<IssueViewAddViewButton baseUrl={baseUrl} />
</TitleWrapper>
}
>
Comment on lines +262 to 269
Copy link
Member Author

@MichaelSun48 MichaelSun48 Mar 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is moved in from navigation.tsx

{views.map(view => (
<AnimatePresence key={view.id} mode="sync">
<Reorder.Group
as="div"
axis="y"
values={views}
onReorder={newOrder => setViews(newOrder)}
initial={false}
ref={sectionRef}
>
{views.map(view => (
<IssueViewNavItemContent
key={view.id}
view={view}
sectionRef={sectionRef}
isActive={view.id === viewId}
Expand All @@ -278,10 +289,9 @@ export function IssueViewNavItems({
isDragging={isDragging}
setIsDragging={setIsDragging}
/>
</AnimatePresence>
))}
<IssueViewAddViewButton baseUrl={baseUrl} />
</Reorder.Group>
))}
</Reorder.Group>
</SecondaryNav.Section>
);
}

Expand All @@ -299,3 +309,9 @@ export const constructViewLink = (baseUrl: string, view: IssueView) => {
},
});
};

const TitleWrapper = styled('div')`
display: flex;
align-items: center;
justify-content: space-between;
`;
66 changes: 32 additions & 34 deletions static/app/views/issues/navigation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,41 +61,39 @@ export function IssueNavigation({children}: IssuesWrapperProps) {
</SecondaryNav.Item>
</SecondaryNav.Section>
{groupSearchViews && (
<SecondaryNav.Section title={t('Starred Views')}>
<IssueViewNavItems
loadedViews={groupSearchViews.map(
(
{
id,
name,
query: viewQuery,
querySort: viewQuerySort,
environments: viewEnvironments,
projects: viewProjects,
timeFilters: viewTimeFilters,
isAllProjects,
},
index
): IssueView => {
const tabId = id ?? `default${index.toString()}`;
<IssueViewNavItems
loadedViews={groupSearchViews.map(
(
{
id,
name,
query: viewQuery,
querySort: viewQuerySort,
environments: viewEnvironments,
projects: viewProjects,
timeFilters: viewTimeFilters,
isAllProjects,
},
index
): IssueView => {
const tabId = id ?? `default${index.toString()}`;

return {
id: tabId,
key: tabId,
label: name,
query: viewQuery,
querySort: viewQuerySort,
environments: viewEnvironments,
projects: isAllProjects ? [-1] : viewProjects,
timeFilters: viewTimeFilters,
isCommitted: true,
};
}
)}
sectionRef={sectionRef}
baseUrl={baseUrl}
/>
</SecondaryNav.Section>
return {
id: tabId,
key: tabId,
label: name,
query: viewQuery,
querySort: viewQuerySort,
environments: viewEnvironments,
projects: isAllProjects ? [-1] : viewProjects,
timeFilters: viewTimeFilters,
isCommitted: true,
};
}
)}
sectionRef={sectionRef}
baseUrl={baseUrl}
/>
)}
<SecondaryNav.Section title={t('Configure')}>
<SecondaryNav.Item
Expand Down
Loading