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

UX: Better composer hyperlink modal #8160

Merged
merged 3 commits into from
Oct 8, 2019
Merged

UX: Better composer hyperlink modal #8160

merged 3 commits into from
Oct 8, 2019

Conversation

pmusaraj
Copy link
Contributor

@pmusaraj pmusaraj commented Oct 4, 2019

This brings the hyperlink modal in line with all other modals (upload, date, etc.) by placing the fixed modal element outside of the composer.

Before:

image

After:

image

@discoursebot
Copy link

You've signed the CLA, pmusaraj. Thank you! This pull request is ready for review.

@@ -0,0 +1,47 @@
import ModalFunctionality from "discourse/mixins/modal-functionality";
import { default as computed } from "ember-addons/ember-computed-decorators";
Copy link
Contributor

Choose a reason for hiding this comment

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

ESLint error, computed is no longer used: https://travis-ci.org/discourse/discourse/jobs/593993307#L701-L702

@SamSaffron
Copy link
Member

I support this cleanup, feel free to merge!

@SamSaffron SamSaffron added the 👍 OP to merge PR author can go ahead and merge! label Oct 8, 2019
@pmusaraj pmusaraj merged commit 30cda17 into discourse:master Oct 8, 2019
@pmusaraj pmusaraj deleted the refactor-composer-hyperlink branch October 8, 2019 20:19
@CvX
Copy link
Contributor

CvX commented Oct 9, 2019

@pmusaraj It seems it's no longer possible to submit the form by pressing enter/return. Could this PR be the cause?

@discoursereviewbot
Copy link

Penar Musaraj posted:

Yes, it’s possible. I think our modals in general do not submit on return, I think we should fix that globally.

@discoursereviewbot
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👍 OP to merge PR author can go ahead and merge!
Development

Successfully merging this pull request may close these issues.

5 participants