Skip to content

Conversation

@patschi
Copy link
Member

@patschi patschi commented Jun 1, 2017

Simply replaced the monitoring endpoint url with an input field and added a listener for automatically selecting the whole url when clicking on the input field.

image

Should (partially) solve #90 point 4, clipboard copy functionality may still be missing.

@Noodlesalat
Copy link
Member

Hey @patschi ,
do you still want to integrate the clipboard copy functionality? I found a example in the Nextcloud Server code but it did not worked out oft the box for me. Link
If you do not habe the time to add this I would try to do so by forking your repo and working on a pull request.

patschi added 5 commits July 18, 2018 22:29
If there's the default margin-bottom:0 applied, the page starts flickering when the "Copy" tooltip is shown. So this is probably sort of a (working) workaround.

Signed-off-by: Patrik Kernstock <[email protected]>
Signed-off-by: Patrik Kernstock <[email protected]>
@patschi
Copy link
Member Author

patschi commented Jul 18, 2018

Thanks a lot for pointing that out, @Noodlesalat! I have just pushed some new changes, including the clipboard copy feature.

Now it looks like:
image

<?php p($l->t('You can connect an external monitoring tool by using this end point:')); ?>
</p>
<div>
<input type="text" readonly="readonly" style="width: 415px;" id="monitoring-endpoint-url" value="<?php echo p($_['ocs']); ?>" />
Copy link
Member

@Noodlesalat Noodlesalat Jul 19, 2018

Choose a reason for hiding this comment

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

style="width: 415px;"
This style tag breaks on mobile devices. I think a value in percent (e.g. 70%) is better. In addition we could move this into the style.css file for better overview.

Copy link
Member Author

Choose a reason for hiding this comment

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

Very good points, thanks! 80% works out great on a few tests, I think it's a good value IMO.

@Noodlesalat
Copy link
Member

This looks good for me now. In my opinion this can be merged! @MorrisJobke
Thanks @patschi for your work!

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke MorrisJobke merged commit 7629da7 into nextcloud:master Aug 20, 2018
@MorrisJobke MorrisJobke added this to the Nextcloud 14 milestone Aug 20, 2018
@rullzer rullzer mentioned this pull request Aug 24, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants