-
Notifications
You must be signed in to change notification settings - Fork 130
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
Custom Insert Module #538
Custom Insert Module #538
Conversation
Co-authored-by: Cess <cessmbuguar@gmail.com>
adds .vscode in .gitignore adds _SpecRunner.html in .gitignore Co-authored-by: Emily Ashley <15912063+emilyashley@users.noreply.github.com> Co-authored-by: Govind Goel <52847415+govindgoel@users.noreply.github.com> Co-authored-by: Cess <cessmbuguar@gmail.com>
@jywarren @Shulammite-Aso @shreyaa-sharmaa @NitinBhasneria This is the initial design. How is it? |
This looks great! I think we'll need to include maps in the options of things to add too, and show another UI for indicating lat, lon and preset layers. And maybe enable search suggestions? But one thing at a time anyway. great! |
I think it would be better to have a different module for maps as inputs would be different for it. What do you think about this? @jywarren @cesswairimu @ebarry @emilyashley |
I have added the functionality Please have a look!. @Shulammite-Aso @shreyaa-sharmaa @NitinBhasneria |
…into CustomInsert
…into CustomInsert
@jywarren Can you please guide me on how to implement the search tag function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really great. In terms of UI design, i was wondering if you thought a modal would be better, especially for use on mobile devices? The HTML builder markup could be included into a Bootstrap modal, what do you think?
Thanks for input @Shulammite-Aso i agree we should think on maps but also agree we can do it later and one thing at a time makes sense.
Finally, i wanted to suggest that the HTML builder template code be kept very modular (like in a function?), because I think we can re-use it upstream in our woofmark
clone. We don't have to worry about this in this PR, but imagine - what if the HTML equivalent of [notes:_____]
were actually the builder HTML form itself. So that you'd see something like:
Power tag: [notes:________] (edit)
and when you click edit
you get this set of form configurable options. When you click Save again, it transforms the whole form back into the [notes:_______]
markdown format. I know this is a bit of a leap but if we build that conversion HTML>Markdown>HTML into our fork of woofmark
we could achieve this! Does that make sense to folks? Pls ask questions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing I'd like to ask for is some tests to protect this functionality at a basic level. Like, to click the button, select a type,and insert it, and confirm that the correct markdown appears. Would that be possible, @keshav234156 ?
Ok,tests are possible to implement here.@shreyaa-sharma is already doing a great work at #541 of integrating jest-puppeteer.We can surely have test after #541 is merged. |
I think modal would be better especially for small screens. Should I try implementing it. |
Oh that'll be awesome. Shall we wait until we can use Jest then? 🎉 Let's hold a little on search for tags - in the spirit of one thing at a time at least for this PR. Do you want to try for a modal based design here? Either with code, or potentially working really quickly with a rough sketch or with https://www.layoutit.com/ ? |
(and if we're going to wait let's mark this title as "pending Jest tests in #541"? |
@jywarren I tried implementing Bootstrap modal (https://getbootstrap.com/docs/3.4/javascript/#modals). But it didn't work as when modal open then pointer leaves the editor and so it's not able to insert required text |
Hmm. Are you able to programmatically re-insert the pointer? I guess it may be hard to re-insert it at the right place. What if we took a different approach - we started by inserting the module, but it contained its own interface for configuring (using the HTML->Markdown interpretation). Then you'd see it inline, and be able to configure it as it is, inside the text document? Maybe something like this: Although it could be more fully featured and larger than the popup we'd had |
Hi! I think we are ready for this to get a Jest test now! After #541 is now complete! |
@jywarren I have added the test. |
@Shulammite-Aso @Shreyaa-s @NitinBhasneria please review!! |
One small request - could there be a space after "Notes" and "List", to space them apart from the carets/arrows? Thank you! |
builder += '<span class="caret"></span>'; | ||
builder += '</button>'; | ||
builder += '<ul class="dropdown-menu" role="menu" aria-labelledby="dropdownMenu1" id="menu">'; | ||
builder += '<li role="presentation"><a role="menuitem" tabindex="-1" id="Notes">Notes</a></li>'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I wonder if it may be better to use a classname so as not to have to use a more unique id
, and to nest the selectors, so you'd be able to call: . woofmark-command-insert .notes
instead of relying on a completely unique Notes
id? And similarly for other id
s here! What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Making changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying to change id into class. How would that be beneficial? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, if you use an id, then if there are 2 editors on the page, they're no longer unique, and thats a rule with ids. It may be better to use nested classes for specificity. This may not come up much, but in those edge cases it's a good practice. And, the id
"menu" is not very unique so it already may collide with id
s in other code. So i guess I'm just noting that we could bypass the uniqueness thing entirely by relying more on classes. Thanks!
Almost there! I see a question or two still needing addressing, then let's resolve conflicts and we should be able to merge this! |
Moved to #568 |
concerns #419
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
grunt jasmine
fixes #0000
-style reference to original issue #@publiclab/reviewers
for help, in a comment belowIf tests do fail, click on the red
X
to learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.
Thanks!