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

ListSettingsView: Append the new field instead of prepend #12587

Merged
merged 4 commits into from Feb 22, 2022

Conversation

gu-stav
Copy link
Contributor

@gu-stav gu-stav commented Feb 18, 2022

What does it do?

When a new field gets added to the list-view of the content manger, it will be prepended to the existing list. Since the add button is on the right and it is supposed to add the item, this PR changes the behavior so that the item is appended instead.

Before

Kapture.2022-02-18.at.14.06.19.mp4

After

Kapture.2022-02-18.at.14.04.11.mp4

Why is it needed?

Makes it easier for the user the predict the behavior of the UI.

How to test it?

  1. Navigate to the content manger
  2. Edit the fields displayed by default
  3. Add a new field

@gu-stav gu-stav added issue: enhancement Issue suggesting an enhancement to an existing feature source: core:content-manager Source is core/content-manager package labels Feb 18, 2022
@gu-stav gu-stav added this to the 4.1.1 milestone Feb 18, 2022
@codecov
Copy link

codecov bot commented Feb 18, 2022

Codecov Report

Merging #12587 (73b063c) into master (393909c) will increase coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12587      +/-   ##
==========================================
+ Coverage   47.59%   47.67%   +0.08%     
==========================================
  Files         210      228      +18     
  Lines        8192     8453     +261     
  Branches     1857     1896      +39     
==========================================
+ Hits         3899     4030     +131     
- Misses       3531     3641     +110     
- Partials      762      782      +20     
Flag Coverage Δ
front ∅ <ø> (∅)
unit 47.67% <ø> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.github/actions/check-pr-status/index.js 91.30% <0.00%> (ø)
...ackages/generators/generators/lib/plops/service.js 57.14% <0.00%> (ø)
packages/generators/generators/lib/plops/policy.js 57.14% <0.00%> (ø)
packages/generators/generators/lib/plops/api.js 21.73% <0.00%> (ø)
...ages/generators/generators/lib/plops/controller.js 55.55% <0.00%> (ø)
...ages/generators/generators/lib/plops/middleware.js 57.14% <0.00%> (ø)
packages/generators/generators/lib/plops/plugin.js 40.00% <0.00%> (ø)
...erators/lib/plops/prompts/bootstrap-api-prompts.js 100.00% <0.00%> (ø)
...es/generators/generators/lib/plops/content-type.js 56.89% <0.00%> (ø)
...rators/lib/plops/prompts/get-attributes-prompts.js 14.28% <0.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 393909c...73b063c. Read the comment docs.

@ronronscelestes
Copy link
Contributor

Nice @gu-stav
Just a question: I think I've codded it this way during v4 with design approval because when you have a long list of fields inside the input - overflowing the input's size - then you don't see the new field added. Because to see it you have to scroll to the right end of the input. Just to be sure it's the behavior we want!
cc @maevalienard

@gu-stav
Copy link
Contributor Author

gu-stav commented Feb 18, 2022

@ronronscelestes Oh, nice catch. That's a valid concern. Well - how about, once you add a field we always scroll to the end of the container afterwards?

@ronronscelestes
Copy link
Contributor

@ronronscelestes Oh, nice catch. That's a valid concern. Well - how about, once you add a field we always scroll to the end of the container afterwards?

@gu-stav I wasn't sure how to do that, that would be great, can't wait to see and learn 👀

@gu-stav
Copy link
Contributor Author

gu-stav commented Feb 18, 2022

@ronronscelestes I've added a proposal in 646653d. Sorry for the diff 🤔 In DraggableCard the only relevant change is to forward the ref.

What do you think about it?

@gu-stav gu-stav marked this pull request as draft February 21, 2022 08:46
@gu-stav
Copy link
Contributor Author

gu-stav commented Feb 21, 2022

@ronronscelestes @soupette Thanks for your review comments. They made me think that a simpler implementation is needed, which I pushed in 6f06b38.

  • we now store the last action that was run (either "add" or "remove"), so that it's clear the effect is not run after a field was removed
  • I removed the refMap implementation on the fields and went back to scrollLeft since it's easier to understand, we don't need the ref on DraggableCard and we should never scroll vertically (which .scrollIntoView() would do, in case the page would be longer)

I hope you like it :)

@gu-stav gu-stav marked this pull request as ready for review February 21, 2022 15:37
@soupette soupette removed the request for review from ronronscelestes February 22, 2022 10:14
@@ -3467,14 +3467,14 @@ exports[`ADMIN | CM | LV | Configure the view should add field 1`] = `
<span
class="c24 c77"
>
michka
hey
Copy link
Contributor

Choose a reason for hiding this comment

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

🥲

@gu-stav gu-stav merged commit 71d55dc into master Feb 22, 2022
@gu-stav gu-stav deleted the fix/list-settings-order branch February 22, 2022 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: enhancement Issue suggesting an enhancement to an existing feature source: core:content-manager Source is core/content-manager package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants