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

formatting buttons in edit project text editor #3633

Merged
merged 3 commits into from Mar 1, 2018

Conversation

@aasis21
Copy link
Contributor

@aasis21 aasis21 commented Feb 18, 2018

#3629
Now, the formatting buttons now show its corresponding rst on the editor.

@davidfischer davidfischer self-requested a review Feb 21, 2018
@davidfischer davidfischer self-assigned this Feb 21, 2018
@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Feb 21, 2018

Firstly, thank you very much for the first-time contribution!

This does in fact fix the RST editor and it worked for me. However, I had a question about the minified file jquery.markitup.pack.js. Did you generate this file? I don't see it when I download the editor from http://markitup.jaysalvat.com. I'm hesitant to include minified JavaScript until I can verify the source.

Ideally, this plugin should come from bower or NPM but perhaps it's best to merge this PR once we've verified the pack file and then switch to loading it from bower in a separate PR.

Edit: bower or NPM

Loading

Copy link
Contributor

@davidfischer davidfischer left a comment

I just had the above question but this PR is very close.

Loading

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Feb 22, 2018

I'm even starting to wonder whether we need this description field at all. It only shows up on project pages and isn't really used to do anything.

Loading

@aasis21
Copy link
Contributor Author

@aasis21 aasis21 commented Feb 23, 2018

Thanks for the review.

Regarding pack file, I did it online using the actual jquery.markitup.js file.

Loading

@aasis21
Copy link
Contributor Author

@aasis21 aasis21 commented Feb 23, 2018

Yes, the significance of description field is another design question.

Loading

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Feb 26, 2018

I'd be +1 on removing it. It's a bunch of code for a small field, which most people will just be copying from a README file or similar.

Sorry to invalidate the work you did @aasis21, but if you could just remove the files, I'd be happy to commit it! Thanks for making the effort, allowing us to have this discussion.

Loading

@aasis21
Copy link
Contributor Author

@aasis21 aasis21 commented Feb 26, 2018

No problems @ericholscher, I'd be happy to do that. So, what you are saying is to completely remove the description feature from the project. Am I right?

Loading

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Feb 27, 2018

I think this PR should be scaled back to just remove the markitup dependency. We can keep a description field for now. Removing this will be a larger chunk of work and requires us to make a design decision around this. We'll want to review the UI changes that this entails for before just removing.

I've have been wanting to get rid of this field for a long time however. I don't think we need it, or any project description on our dashboard page -- this isn't a page that readers see.

I've opened #3689 for core to discuss removing the field.

Loading

Copy link
Contributor

@agjohnson agjohnson left a comment

I'm glad to get rid of the markitup dependency! For now, let's scale this back to just remove the markitup dependency and once we have more guidance on removing the description field we can work on that.

Loading

<link rel="stylesheet" type="text/css" href="{{ MEDIA_URL }}lib/markitup/sets/sphinx/editor.css" />
{% endblock %}

{% block dash-nav-create %}active{% endblock %}
Copy link
Contributor

@agjohnson agjohnson Feb 27, 2018

Choose a reason for hiding this comment

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

This is a required line.

Loading

Copy link
Contributor Author

@aasis21 aasis21 Feb 27, 2018

Choose a reason for hiding this comment

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

Sorry, it was removed by mistake. Thanks for pointing out.

Loading

@aasis21 aasis21 force-pushed the fix-broken-text-editor branch from 8266808 to 1330fd3 Feb 27, 2018
@aasis21
Copy link
Contributor Author

@aasis21 aasis21 commented Feb 27, 2018

Thanks, @agjohnson for clarifying the situation. I have made changes as you suggested.

Loading

Copy link
Contributor

@davidfischer davidfischer left a comment

I'm glad to have code removed. LGTM!

Loading

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Mar 1, 2018

Thanks so much for this PR @aasis21! I love PR's that delete code, and this one deletes a lot :)

Loading

@ericholscher ericholscher merged commit 308879a into readthedocs:master Mar 1, 2018
1 check passed
Loading
@aasis21
Copy link
Contributor Author

@aasis21 aasis21 commented Mar 1, 2018

Thanks @ericholscher

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants