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.

Sign up for GitHub

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

Jump to bottom

Add newFileMenu and refactor library with rollup #420

Merged
merged 2 commits into from Jun 16, 2022
Merged

Add newFileMenu and refactor library with rollup #420

merged 2 commits into from Jun 16, 2022

Conversation

skjnldsv
Copy link
Contributor

@skjnldsv skjnldsv commented Jun 9, 2022 •
edited

@nextcloud/server

@skjnldsv skjnldsv changed the title Add newFileMenu and refactor library Add newFileMenu and refactor library with rollup Jun 9, 2022
@skjnldsv skjnldsv added the enhancement New feature or request label Jun 9, 2022
@codecov

This comment was marked as resolved.

Sign in to view
artonge
artonge requested changes Jun 14, 2022
View reviewed changes
lib/newFileMenu.ts Show resolved Hide resolved
lib/newFileMenu.ts Show resolved Hide resolved
@skjnldsv skjnldsv requested a review from artonge June 14, 2022 16:15
ChristophWurst
ChristophWurst reviewed Jun 15, 2022
View reviewed changes
package.json Outdated Show resolved Hide resolved
ChristophWurst
ChristophWurst reviewed Jun 15, 2022
View reviewed changes
Copy link
Contributor

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Merge this PR if you must, but then I'm fully removing myself from this package

.github/workflows/fixup.yml Outdated Show resolved Hide resolved
@ChristophWurst
Copy link
Contributor

Is there a ticket we could reference for anyone who is not familiar with newFileMenu?

CarlSchwan
CarlSchwan reviewed Jun 15, 2022
View reviewed changes
__tests__/newFileMenu.spec.ts Show resolved Hide resolved
jest.config.js Show resolved Hide resolved
lib/index.ts Outdated Show resolved Hide resolved
lib/newFileMenu.ts Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@skjnldsv skjnldsv force-pushed the feat/newFileMenu-refactor branch 4 times, most recently from 9ae7077 to 27f31f7 Compare June 15, 2022 17:53
@skjnldsv
Copy link
Contributor Author

Here we go, I removed as much as unrelated things I could find @ChristophWurst
I will create other PR for the workflows, and maybe another one for babel if I have time :)

CarlSchwan
CarlSchwan reviewed Jun 15, 2022
View reviewed changes
package.json Outdated Show resolved Hide resolved
@skjnldsv
Add newFileMenu and refactor library
7c1cd70 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv skjnldsv requested a review from CarlSchwan June 16, 2022 05:54
@skjnldsv
Better documentation
bfeb49c 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv skjnldsv merged commit 32af081 into master Jun 16, 2022
@skjnldsv skjnldsv deleted the feat/newFileMenu-refactor branch June 16, 2022 16:53
skjnldsv
skjnldsv commented Jun 17, 2022
View reviewed changes
lib/newFileMenu.ts
iconSvgInline?: string
/** Existing icon css class */
iconClass?: string
fileType: FileType
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@juliushaertl @PVince81 any of you know why we actually use that property?
It has been like that for years
https://github.com/nextcloud/server/blob/ac2bc2384efe3c15ff987b87a7432bc60d545c67/apps/files/js/newfilemenu.js#L204
https://github.com/nextcloud/server/blob/ac2bc2384efe3c15ff987b87a7432bc60d545c67/apps/files/js/newfilemenu.js#L110

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to be a leftover from how we used to manage creation
owncloud/core@d386168#diff-4e16996e525fd1d3ef643308e0c2edbd87c0776afb54b426b3a21cad6fca1868L181

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 guess we can drop this.
The only thing we need is icon, text, new file text string, ID and handler, no?

ChristophWurst
ChristophWurst reviewed Sep 27, 2022
View reviewed changes
jest.config.js
},
},
transformIgnorePatterns: [
'node_modules/(?!(abcde|fghij)/)',
Copy link
Contributor

Choose a reason for hiding this comment

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

🧐

@skjnldsv skjnldsv mentioned this pull request Nov 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst left review comments

@blizzz blizzz blizzz approved these changes

@artonge artonge artonge approved these changes

@szaimen szaimen Awaiting requested review from szaimen

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@skjnldsv @ChristophWurst @blizzz @artonge @CarlSchwan