Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

client/web: Fix regression in connected accounts for ADO#48184

Merged
indradhanush merged 1 commit into
mainfrom
ig/ado-connected-account-ui-regression
Feb 24, 2023
Merged

client/web: Fix regression in connected accounts for ADO#48184
indradhanush merged 1 commit into
mainfrom
ig/ado-connected-account-ui-regression

Conversation

@indradhanush

@indradhanush indradhanush commented Feb 24, 2023

Copy link
Copy Markdown
Contributor

Missed this edge case when I was addressing code review comments in #47865.

Test plan

  • Tested locally

Before

image

After

image

image

  • Builds should pass

App preview:

Check out the client app preview documentation to learn more.

@cla-bot cla-bot Bot added the cla-signed label Feb 24, 2023
@indradhanush indradhanush requested a review from a team February 24, 2023 12:06
@indradhanush indradhanush self-assigned this Feb 24, 2023
@indradhanush indradhanush added bug An error, flaw or fault that produces an incorrect or unexpected result, or behavior. ADO Tier 1 support for Azure DevOps labels Feb 24, 2023
@sg-e2e-regression-test-bob

Copy link
Copy Markdown

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (0.00 kb) 0.00% (+0.07 kb) 0.00% (+0.07 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits aa32a80 and e5912f4 or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@sashaostrikov sashaostrikov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! left 1 question

accountConnection = (
<>
{account.external?.displayName} (@{account.external?.login})
{account.external?.displayName ? (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

question: can empty tags on lines 56 and 64 be removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's not an empty tag. It's wrapping the whole thing in an empty component without which the code does not compile.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe there's an elegant way to do this? I'm a tsx noob. :P

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know that this is an empty component, but it's still a tag :)
okay, I thought in this case it could have been omitted

@indradhanush indradhanush merged commit f3b5a93 into main Feb 24, 2023
@indradhanush indradhanush deleted the ig/ado-connected-account-ui-regression branch February 24, 2023 12:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

ADO Tier 1 support for Azure DevOps bug An error, flaw or fault that produces an incorrect or unexpected result, or behavior. cla-signed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants