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 mindmap type file icon #19942

Merged
merged 5 commits into from
Dec 28, 2020
Merged

Add mindmap type file icon #19942

merged 5 commits into from
Dec 28, 2020

Conversation

ACTom
Copy link
Contributor

@ACTom ACTom commented Mar 13, 2020

No description provided.

ACTom referenced this pull request in ACTom/files_mindmap Mar 13, 2020
@ACTom
Add icon to mimetype mindmap
ad802f6
@ACTom ACTom force-pushed the add-mindmap-icon branch from 2710bd0 to 81ebf52 Compare March 13, 2020 19:40
ACTom
ACTom commented Mar 14, 2020
View reviewed changes
Copy link
Contributor Author

@ACTom ACTom left a comment

Choose a reason for hiding this comment

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

Just add a svg file

@ACTom ACTom mentioned this pull request Mar 14, 2020
Open
@kesselb
Copy link
Contributor

kesselb commented Mar 14, 2020

Thanks 👍

Are you the creator of the icon?

@ACTom
Copy link
Contributor Author

ACTom commented Mar 14, 2020

Yes, but I refer to this picture (https://marinettedognanny.com/wp-content/uploads/2018/10/social_icon.png)

@kesselb
Copy link
Contributor

kesselb commented Mar 14, 2020

cc @jancborchardt @schiessle

I'm not sure if we are able to use that icon. Mind to have a look for a icon available under MIT license? For example: https://feathericons.com

@kesselb kesselb added 3. to review Waiting for reviews enhancement labels Mar 14, 2020
@kesselb kesselb added this to the Nextcloud 19 milestone Mar 14, 2020
This was referenced Apr 4, 2020
Merged
Merged
@ACTom
Copy link
Contributor Author

ACTom commented Apr 14, 2020

@kesselb I used Inkscape to redraw this icon, and now I can confirm that this icon was made by me.

By the way, if I want to add mindmap type in core / js / mimetypelist.js, is it better to submit a new PR, or to submit it directly in this PR?

@ACTom ACTom force-pushed the add-mindmap-icon branch from 5b10093 to fc675ba Compare April 14, 2020 15:15
@kesselb
Copy link
Contributor

kesselb commented Apr 14, 2020

Thanks. Please submit another pr for the mime type.

@ACTom
Copy link
Contributor Author

ACTom commented Apr 15, 2020

@kesselb I see that this PR does not have reviewers yet, what do I need to do?

This was referenced Apr 15, 2020
Merged
Merged
jancborchardt
jancborchardt approved these changes Apr 16, 2020
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Could look a but more geometric and the lines are a bit thin, but seems fine.

Any particular reason for the choice of color? Ideally color always means something or is related to e.g. a popular product (like red for PDF).

@ACTom
Copy link
Contributor Author

ACTom commented Apr 16, 2020

Any particular reason for the choice of color? Ideally color always means something or is related to e.g. a popular product (like red for PDF).

There is no special reason. This color is chosen because it looks good, and it is different from the colors of other existing formats. You can clearly identify the file type in the list.

@ACTom
Copy link
Contributor Author

ACTom commented Apr 17, 2020

@juliushaertl Waiting for your review.

@kesselb
Copy link
Contributor

kesselb commented Apr 17, 2020 •
edited
Loading

@ACTom https://twitter.com/frankdejonge/status/1236727595670134786 :)

It's already queued for 19 and will be reviewed in time.

@ACTom
Copy link
Contributor Author

ACTom commented Apr 17, 2020

@kesselb Oh, I'm sorry, thanks.

@rullzer rullzer mentioned this pull request Apr 23, 2020
Merged
11 tasks
@jancborchardt jancborchardt requested a review from skjnldsv April 23, 2020 08:32
@skjnldsv
Copy link
Member

Why do we need this icon in server?

@kesselb
Copy link
Contributor

kesselb commented Apr 23, 2020 •
edited
Loading

For mindmap files :) Is there a way to add this image as app?

@skjnldsv
Copy link
Member

For mindmap files :) Is there a way to add this image as app?

No idea, but can't you not register mimetypes and provide your own img?

@ACTom
Copy link
Contributor Author

ACTom commented Oct 14, 2020

Try: https://gh.neting.ccmunity/t/update-a-forked-repository-when-the-original-repository-is-updated/1855

(don't forget to backup your work just in case)

Thanks, but where to fix Test\IntegrityCheck\CheckerTest::testVerifyCoreSignatureWithModifiedMimetypelistSignatureData fails? It need to modify https://github.com/nextcloud/server/blob/master/tests/lib/IntegrityCheck/CheckerTest.php .

@ACTom
Copy link
Contributor Author

ACTom commented Oct 15, 2020

@kesselb I have push a new PR to fix Test\IntegrityCheck\CheckerTest::testVerifyCoreSignatureWithModifiedMimetypelistSignatureData fails #23463

@kesselb
Copy link
Contributor

kesselb commented Oct 15, 2020

But the test does not fail on master.

  1. Rebase this branch and pull the latest changes from master (this repository not your fork)
  2. Merge your other patch into this branch

Then we can review in one go and also have a recent CI run.

@ACTom
Copy link
Contributor Author

ACTom commented Oct 15, 2020

But the test does not fail on master.
No, the fail exists on the master and has nothing to do with this PR, you can try it

@kesselb
Copy link
Contributor

kesselb commented Oct 15, 2020

I don't see the test failing on a recent CI run: https://drone.nextcloud.com/nextcloud/server/34193

But ignore this for now and rebase this branch to have a recent CI run.

@ACTom
Copy link
Contributor Author

ACTom commented Oct 17, 2020

I don't see the test failing on a recent CI run: https://drone.nextcloud.com/nextcloud/server/34193

But ignore this for now and rebase this branch to have a recent CI run.
@kesselb
I don't know if the link test set you gave is complete, but I pull the master code and execute ./autotest.sh or phpunit --bootstrap tests/bootstrap.php tests/lib/IntegrityCheck/CheckerTest.php then all reported that error, so I opened #23463 to fix it.

@kesselb
Copy link
Contributor

kesselb commented Oct 17, 2020

Please rebase this branch to have a recent CI run.

ACTom added 2 commits October 17, 2020 23:40
@ACTom
Add mindmap type file icon
5ef9f7e 
Signed-off-by: ACTom <i@actom.me>
@ACTom
Modify mindmap type file icon
e0aa367 
Signed-off-by: ACTom <i@actom.me>
@kesselb kesselb added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 17, 2020
@ACTom ACTom mentioned this pull request Nov 6, 2020
Open
@ACTom
Copy link
Contributor Author

ACTom commented Nov 14, 2020

@ChristophWurst @rullzer Can you review it?

@szaimen
Copy link
Contributor

szaimen commented Dec 21, 2020

Ping @rullzer

ChristophWurst
ChristophWurst approved these changes Dec 21, 2020
View reviewed changes
Copy link
Member

@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.

Fine by me

tests/lib/IntegrityCheck/CheckerTest.php Outdated Show resolved Hide resolved
@ChristophWurst ChristophWurst added this to the Nextcloud 21 milestone Dec 21, 2020
@rullzer rullzer mentioned this pull request Dec 22, 2020
Merged
39 tasks
@ACTom
Readd test getOnlyDefaultAliases()
d9db6ae 
Signed-off-by: ACTom <i@actom.me>
@ChristophWurst ChristophWurst merged commit b981cb0 into nextcloud:master Dec 28, 2020
@szaimen
Copy link
Contributor

szaimen commented Dec 28, 2020

🎉

@rullzer rullzer mentioned this pull request Dec 28, 2020
Merged
39 tasks
@nursoda
Copy link

nursoda commented Jan 15, 2021

Wait, there's a patch for NC 19 and 21 but not for 20? Or did I overlook something?

@kesselb
Copy link
Contributor

kesselb commented Jan 15, 2021

21

@skjnldsv
Copy link
Member

/backport to stable20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szaimen szaimen szaimen left review comments

@kesselb kesselb kesselb left review comments

@skjnldsv skjnldsv skjnldsv approved these changes

@juliusknorr juliusknorr juliusknorr approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@georgehrke georgehrke georgehrke approved these changes

@jancborchardt jancborchardt jancborchardt approved these changes

@rullzer rullzer Awaiting requested review from rullzer

Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Milestone
Nextcloud 21
Development

Successfully merging this pull request may close these issues.
10 participants
@ACTom @kesselb @skjnldsv @juliusknorr @ChristophWurst @szaimen @nursoda @rullzer @jancborchardt @georgehrke