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

[student-libraries] Allow renaming a library when creating/publishing it #32599

Merged
merged 5 commits into from
Jan 14, 2020

Conversation

jmkulwik
Copy link
Contributor

@jmkulwik jmkulwik commented Jan 9, 2020

Description

OLD:
image

NEW:
image

Links

Testing story

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@jmkulwik jmkulwik requested a review from a team as a code owner January 9, 2020 21:49
"libraryExportNoCommentError": "This function cannot be exported until you add a comment to it.",
"libraryExportTitle": "Export Functions as a Library",
"libraryName": "Library Name:",
"libraryName": "Library Name",
"libraryNameRequirements": "Your library's name must start with a capital letter and use only letters, numbers, and underscores.",
Copy link
Contributor

Choose a reason for hiding this comment

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

note that many languages have no concept of capital letters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this came from a desire to teach good naming conventions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh you mean written/spoken languages - not programming languages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point. I'll circle back with curriculum and prod on that one.

Copy link

@epeach epeach left a comment

Choose a reason for hiding this comment

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

LGTM

export function suggestName(libraryName) {
let suggestedName = sanitizeName(libraryName);
if (suggestedName.length === 0 || !isNaN(suggestedName.charAt(0))) {
suggestedName = 'Lib' + suggestedName;
Copy link

Choose a reason for hiding this comment

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

Is this okay for non-English libraries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm great question. I'll circle back with the curriculum & prod teams on that one.

@jmkulwik
Copy link
Contributor Author

RE: Comments regarding internationalization. I've started a thread with curriculum & prod, but I don't think this should block this PR because as these issues are not introduced by this PR and will be straightforward to update/fix when answers come in. I've scheduled that work as followup.
Internal folks can see that item here: https://codedotorg.atlassian.net/browse/STAR-946

@jmkulwik jmkulwik merged commit 40bdb94 into staging Jan 14, 2020
@jmkulwik jmkulwik deleted the library-rename branch January 14, 2020 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants