Skip to content
This repository has been archived by the owner on Jul 18, 2024. It is now read-only.

[BD-14] Library Authoring - Edit Library Title #61

Merged

Conversation

UvgenGen
Copy link
Contributor

Description:
Library title is editable by clicking on it like in Account MFE

@openedx-webhooks
Copy link

Thanks for the pull request, @UvgenGen!

When this pull request is ready, tag your edX technical lead.

@openedx-webhooks openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Aug 25, 2022
@connorhaugh
Copy link
Contributor

Looks good you have some lint fixes to do real quick though.

@UvgenGen
Copy link
Contributor Author

UvgenGen commented Sep 1, 2022

Looks good you have some lint fixes to do real quick though.

@connorhaugh linter fixed)

Copy link
Contributor

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

Tested this locally, this is definitely a great feature to add!

One thing I noticed in testing is that the user must click (or tab) outside of the text box to fire onBlur and apply the change (pressing enter doesn't work). When I first tried to edit a Content Library name I pressed enter and nothing happened.

I'm not sure if that's unique to this field, or if it's a UX oddity that exists throughout the rest of Open edX as well.

@UvgenGen
Copy link
Contributor Author

@brian-smith-tcril hi! Should we add handler for enter button?

@codecov
Copy link

codecov bot commented Sep 12, 2022

Codecov Report

Base: 48.59% // Head: 49.13% // Increases project coverage by +0.54% 🎉

Coverage data is based on head (b20b172) compared to base (5b2c5d3).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head b20b172 differs from pull request most recent head 217235f. Consider uploading reports for the commit 217235f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master      openedx/frontend-wg#61      +/-   ##
==========================================
+ Coverage   48.59%   49.13%   +0.54%     
==========================================
  Files          76       76              
  Lines        1953     1972      +19     
  Branches      353      358       +5     
==========================================
+ Hits          949      969      +20     
+ Misses        970      969       -1     
  Partials       34       34              
Impacted Files Coverage Δ
...-authoring/author-library/LibraryAuthoringPage.jsx 88.38% <100.00%> (+0.95%) ⬆️
...library-authoring/configure-library/data/thunks.js 18.18% <100.00%> (ø)
...ary-authoring/create-library/LibraryCreatePage.jsx 84.04% <0.00%> (ø)
.../library-authoring/common/OrganizationDropdown.jsx 95.23% <0.00%> (+0.23%) ⬆️
...rary-authoring/configure-library/data/selectors.js 100.00% <0.00%> (+33.33%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@brian-smith-tcril
Copy link
Contributor

brian-smith-tcril commented Sep 13, 2022

It seems on the Account MFE the pattern is slightly different.

This PR is creating what I'd describe as an "editable header" or "editable title," which is not something we have a Paragon component for.

The account MFE has

image
image
and pressing enter performs a save (with visual feedback)
image

and uses some paragon components (https://github.com/openedx/frontend-app-account/blob/e01b595a0c4be60f886587c62203b35b70f50f61/src/account-settings/name-change/NameChange.jsx), but it seems there is still room for improvement re: component reusability

This PR has

image
image

Overall thoughts

I'm not sure if any of these UX questions really belong in the scope of this PR. It's definitely worth documenting the flows that we want to have for MFEs. It's also definitely worth creating reusable components to support those flows. I'm just not sure if that should be a blocker for adding this functionality.

Ideally we can determine:

  • What short term changes need to happen to get this PR ready to merge so we can have this functionality added quickly?
  • What long term changes need to happen to support this flow with better code reuse and UX consistency?

@arbrandes
Copy link
Contributor

@brian-smith-tcril, @UvgenGen this MFE is in need of a UX and styling makeover anyway. We should stick to the minimal changes here: adding the enter key handler, for instance.

Longer term, Brian and I are discussing what to do about bringing the MFE up to speed with Paragon (via #71).

@UvgenGen
Copy link
Contributor Author

@arbrandes I've added the enter key handler

@arbrandes arbrandes self-requested a review September 15, 2022 13:20
@connorhaugh
Copy link
Contributor

@arbrandes is there anything specific we need to handle here then?

this MFE is in need of a UX and styling makeover anyway. We should stick to the minimal changes here: adding the enter key handler, for instance.

seems to imply that this PR can be approved.

@arbrandes
Copy link
Contributor

Yeah, we can totally merge this!

@arbrandes arbrandes merged commit cb06f52 into openedx-unsupported:master Oct 17, 2022
@openedx-webhooks
Copy link

@UvgenGen 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blended PR is managed through 2U's blended developmnt program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants