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.

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

client/web/src/site-admin: Add history panel #49842

Merged
merged 8 commits into from Mar 23, 2023

Conversation

indradhanush
Copy link
Contributor

@indradhanush indradhanush commented Mar 22, 2023

👉 Loom: https://www.loom.com/share/d84dfbf54f3c46d99a67ff79910eadbe

Fixes #46032.

Test plan

  1. Tested locally
  2. Builds should pass

App preview:

Check out the client app preview documentation to learn more.

Co-authored-by: Milan Freml <kopancek@users.noreply.github.com>
@indradhanush indradhanush added site-admin Site admin experience site-admin-ux Issues related to site-admin UX: bugs, papercuts, design, ... labels Mar 22, 2023
@indradhanush indradhanush requested review from a team March 22, 2023 13:53
@indradhanush indradhanush self-assigned this Mar 22, 2023
@cla-bot cla-bot bot added the cla-signed label Mar 22, 2023
@sg-e2e-regression-test-bob
Copy link

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (0.00 kb) 0.04% (+6.54 kb) 0.06% (+6.54 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits b249fa6 and ccf9b20 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

Copy link
Contributor

@sashaostrikov sashaostrikov left a comment

Choose a reason for hiding this comment

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

Nice improvement, good job :)
Added some comments.

client/web/src/site-admin/SiteAdminConfigurationPage.tsx Outdated Show resolved Hide resolved
client/web/src/site-admin/SiteAdminConfigurationPage.tsx Outdated Show resolved Hide resolved
client/web/src/site-admin/SiteAdminConfigurationPage.tsx Outdated Show resolved Hide resolved
client/web/src/site-admin/SiteAdminConfigurationPage.tsx Outdated Show resolved Hide resolved
client/web/src/site-admin/SiteAdminConfigurationPage.tsx Outdated Show resolved Hide resolved
client/web/src/site-admin/SiteAdminConfigurationPage.tsx Outdated Show resolved Hide resolved
client/web/src/site-admin/SiteAdminConfigurationPage.tsx Outdated Show resolved Hide resolved
client/web/src/site-admin/SiteAdminConfigurationPage.tsx Outdated Show resolved Hide resolved
pnpm-lock.yaml Show resolved Hide resolved
client/web/src/site-admin/SiteAdminConfigurationPage.tsx Outdated Show resolved Hide resolved
@varsanojidan
Copy link
Contributor

Looks great Indra, I think this is going to be really helpful for Site Admins!

Have you played around with adding colors to the diffs by any chance? Something like this?

I feel like visually it would be kind of nice.

Copy link
Contributor

@sashaostrikov sashaostrikov left a comment

Choose a reason for hiding this comment

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

Did a second round of comments.
Ongoing fixes LGTM, waiting for new commits :)

@indradhanush
Copy link
Contributor Author

Have you played around with adding colors to the diffs by any chance? Something like this?

@varsanojidan I did not. For the first version we decided to ship it without colors to start with.

@indradhanush indradhanush requested a review from a team March 23, 2023 11:41
Without this the loading spinner loads outisde the component and it
looks ugly.
Copy link
Contributor

@sashaostrikov sashaostrikov left a comment

Choose a reason for hiding this comment

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

LGTM!
I think you need to add a horizontal line in case of an empty diff too?
Let's fix it and :shipit:

@indradhanush
Copy link
Contributor Author

I think you need to add a horizontal line in case of an empty diff too?

@sashaostrikov Diff will technically never be empty any more since we changed the backend to skip redundant entries. The API spec allows it to be null at the moment which needs to be changed.

Copy link
Contributor

@sashaostrikov sashaostrikov left a comment

Choose a reason for hiding this comment

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

Approving to unblock.
Nice job!

@indradhanush indradhanush enabled auto-merge (squash) March 23, 2023 12:48
@indradhanush indradhanush merged commit 648dedd into main Mar 23, 2023
17 checks passed
@indradhanush indradhanush deleted the ig/site-config-history-ui branch March 23, 2023 13:03
@danielmarquespt
Copy link
Contributor

danielmarquespt commented Mar 24, 2023

Regarding the avatar:

It would be nice to have the avatars, but not at the expense of performance. If we figure out a way of displaying avatars without a performance hit, then this would be an example of how to show them:

Screenshot 2023-03-24 at 11 50 52

if not:

Screenshot 2023-03-24 at 11 55 03

Nevertheless, I think we could change the title of the section to "Change history" and the layout of each row.

I also think we can be clear about the changes that happen via file instead of via user. I added wording for this, however I'm not entirely sure if it captures the exact scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed site-admin Site admin experience site-admin-ux Issues related to site-admin UX: bugs, papercuts, design, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show site configuration edit history on details page
5 participants