Conversation
akalia-atlassian
requested review from
bwieger-atlassian-com,
cabella-dot,
marcomura and
sdzh-atlassian
as code owners
akalia-atlassian
changed the base branch from
main
to
sn/theme-and-side-panel-revamp
There was a problem hiding this comment.
This file is showing changes because we reverted from 2 to 4 line change in a previous PR and took a merge from it in this one. Ideally, this one should disappear if all the stacked PRs underneath will get merged.
I took a merge because this PR adds tweo dependencies as evidenced if look at it via the rich diff feature of GitHub.
| '.vscode-dark': { | ||
| '--ds-icon-success': '#2ABB7F', | ||
| '--ds-icon-warning': '#F5CD47', | ||
| '--ds-skeleton': '#CECED912', |
There was a problem hiding this comment.
The new components demand this. And because they are translucent shimmers with no VS Code equivalent. So just used Atlassian design values.
There was a problem hiding this comment.
Ideally link these to sources you fetched this from so they can be kept in sync.
Alternatively, why don't we fetch this from the package that stores all named colors?
| </ExpansionPanelSummary> | ||
| <ExpansionPanelDetails>{isLoading ? <CircularProgress /> : children}</ExpansionPanelDetails> | ||
| <ExpansionPanelDetails> | ||
| {isLoading ? <Skeleton width="100%" height="80px" borderRadius={3} /> : children} |
There was a problem hiding this comment.
Changes the loader used.
|
|
||
| const PullRequestSidebar: React.FC<PullRequestSidebarProps> = ({ state, controller, taskTitle }) => { | ||
| const PullRequestSidebar: React.FC<PullRequestSidebarProps> = ({ state, controller }) => { | ||
| const taskSubtitle = useMemo(() => { |
There was a problem hiding this comment.
Refactor: calculating here instead of passing from above.
| return numTasks === 0 ? '0 tasks' : `${numCompletedTasks} of ${numTasks} complete`; | ||
| }, [state.tasks]); | ||
|
|
||
| const buildStatusSubtitle = useMemo(() => { |
There was a problem hiding this comment.
This one is slightly different: Instead of just saying "x of x builds passed", now it says "Builds | x of x passed". This is to mimic how task panel handles it - the 0 builds case should have no such qualification.
| isLoading={state.loadState.buildStatuses} | ||
| isDefaultExpanded | ||
| hidden={state.buildStatuses.length === 0} | ||
| hidden={false} |
There was a problem hiding this comment.
Removed hidden: users will want to know if there is nothing too. Removing the panel is more confusing.
| <Grid item> | ||
| <BasicPanel | ||
| isLoading={state.loadState.basicData} | ||
| isLoading={state.loadState.buildStatuses} |
There was a problem hiding this comment.
More accurate isLoading value: there is no loading for reviewers so using build statuses.
| </Paper> | ||
| </Grid> | ||
| </Grid> | ||
| {state.loadState.basicData ? ( |
There was a problem hiding this comment.
If basicData is not there, it shows a simple, full page loading message.
|
|
||
| prHeaderTitle: { | ||
| flex: '1 0 auto', | ||
| flex: '1 1 auto', |
There was a problem hiding this comment.
Change from a previous PR - important but this PR is not the source of truth for this.
|
|
||
| return ( | ||
| <Grid container direction="column" style={{ padding: '0', width: '100%' }}> | ||
| {activeParticipants.length === 0 ? ( |
There was a problem hiding this comment.
This diff is so big because we removed the "No reviewers!" message (which changed the indent) if there are no reviewers: now it simply shows nothing - we already have an "Add reviewer" button underneath to direct the users towards action.
bwieger-atlassian-com
left a comment
bwieger-atlassian-com
left a comment
There was a problem hiding this comment.
LGTM ✅
Checked out, built, manually test
sn2912
force-pushed
the
sn/theme-and-side-panel-revamp
branch
from
117bb23 to
f94253f
Compare
akalia-atlassian
dismissed
bwieger-atlassian-com’s stale review
The base branch was changed.
|
This PR has been superseded by another existing open PR. This is to reduce the clutter. |
4 participants
What Is This Change?
This PR does two things:
a. Check if the basic data is there. If not, it waits and shows a page wide loading. This avoids awkward Ui state like empty PR id and titles.
b. Once the basic page is loaded, it shows the loaders as subtle skeleton shimmers instead of circular loaders.
How Has This Been Tested?
Old Circular Loaders: https://www.loom.com/share/d78a7ca19ab9430f8a3fa2e5a1ab6999
New Subtle Loaders: https://www.loom.com/share/6017f8612fd14b3889de821f0e7b0bd2
Basic checks:
npm run lintnpm run testAdvanced checks:
Recommendations: