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

Website improvements #345

Merged
merged 64 commits into from
Oct 1, 2020
Merged

Website improvements #345

merged 64 commits into from
Oct 1, 2020

Conversation

ajstewart
Copy link
Contributor

@ajstewart ajstewart commented Sep 23, 2020

Website updates that:

Fixes #118.
Fixes #225.
Fixes #263.
Fixes #274.
Fixes #280.

@ajstewart ajstewart changed the title WIP: Website improvements Website improvements Sep 24, 2020
@ajstewart
Copy link
Contributor Author

@srggrs @marxide I've spent some time tidying up the website and making sure all the information that should be displayed is displayed, among a bunch of other changes in the top comment.

Though I wanted to ask for your help on a couple things:

  • You'll see that I've added a copy-to-clipboard functionality on coordinates, e.g.

    <script type="text/javascript">
    bootstrap_alert = function() {}
    bootstrap_alert.success = function(message) {
    $('#alert_placeholder').html('<div id="copyalert" class="alert alert-success alert-dismissible fade show" role="alert">'+message+'<button type="button" class="close" data-dismiss="alert" aria-label="Close"><span aria-hidden="true">&times;</span></button></div>');
    setTimeout(function () {
    $("#copyalert").fadeTo(500, 0).slideUp(600, function () {
    $(this).remove();
    });
    }, 2000);
    }
    var $body = document.getElementsByTagName('body')[0];
    var $btnCopy = document.getElementById('btnCopy');
    var $btnDegCopy = document.getElementById('btnDegCopy');
    var $btnGalCopy = document.getElementById('btnGalCopy');
    var secretInfo = document.getElementById('secretInfo').innerHTML;
    var secretDegInfo = document.getElementById('secretDegInfo').innerHTML;
    var secretGalInfo = document.getElementById('secretGalInfo').innerHTML;
    var copyToClipboard = function(secretInfo) {
    var $tempInput = document.createElement('INPUT');
    $body.appendChild($tempInput);
    $tempInput.setAttribute('value', secretInfo)
    $tempInput.select();
    document.execCommand('copy');
    $body.removeChild($tempInput);
    }
    $btnCopy.addEventListener('click', function(ev) {
    copyToClipboard(secretInfo);
    bootstrap_alert.success("{{ source.wavg_ra_hms }} {{ source.wavg_dec_dms }} copied!");
    });
    $btnDegCopy.addEventListener('click', function(ev) {
    copyToClipboard(secretDegInfo);
    bootstrap_alert.success("{{ source.wavg_ra|floatformat:6 }} {{ source.wavg_dec|floatformat:6 }} copied!");
    });
    $btnGalCopy.addEventListener('click', function(ev) {
    copyToClipboard(secretGalInfo);
    bootstrap_alert.success("{{ source.wavg_l|floatformat:6 }} {{ source.wavg_b|floatformat:6 }} copied!");
    });
    </script>

    I wanted to ask for general help and feedback on two areas:
    Firstly should this script that I've placed in the html files be placed in its own .js file?
    Second I got some alerts working on a successful copy (which I have taken from my RACS website) but I couldn't work out how to get the alert to look the same as the one that is used on the pipeline run creation.

  • The datatables that are produced by custom viewsets don't work on the sorting or searching. E.g. the related sources table that Sergio made (I have copied this method to create other tables). When I investigate with the console I can see that JS9 is attempting to do something with the request, where as this doesn't happen with tables created with the standard api -list calls. I couldn't work out what was going on, perhaps you'll have an idea @marxide?

Screen Shot 2020-09-24 at 21 10 45

You could go on forever with the website edits but I think this PR will at least now present all the information possible from the pipeline.

@ajstewart ajstewart marked this pull request as ready for review September 24, 2020 11:13
@ajstewart ajstewart requested review from marxide and srggrs September 24, 2020 11:13
@ajstewart
Copy link
Contributor Author

I also couldn't work out how to make the source query perform the search when it finds the hash arguments. I tried to simulate a click on the catalogSearch button but this didn't seem to work.

@marxide
Copy link
Contributor

marxide commented Sep 24, 2020

Firstly should this script that I've placed in the html files be placed in its own .js file?

My opinion is that short, page-specific scripts (i.e. not reused on other pages) like this one are fine as is. The disadvantage is that they won't get minified by gulp, but they're not that long anyway. I don't feel strongly about it though.

Second I got some alerts working on a successful copy (which I have taken from my RACS website) but I couldn't work out how to get the alert to look the same as the one that is used on the pipeline run creation.

I'm not sure I like the pipeline run creation alert. I barely noticed it! I think we should move their placement to top-right or top-centre. There's an open issue to replace the alerts with Bootstrap's toast component #260, maybe we should leave your alert for now and fix it when we replace the others with toasts?

  • The datatables that are produced by custom viewsets don't work on the sorting or searching. E.g. the related sources table that Sergio made (I have copied this method to create other tables). When I investigate with the console I can see that JS9 is attempting to do something with the request, where as this doesn't happen with tables created with the standard api -list calls. I couldn't work out what was going on, perhaps you'll have an idea @marxide?

Oh yikes, I think the JQuery that is bundled in JS9 is taking precedence probably because it's loaded after JQuery. We should definitely fix that - I'll open an issue and investigate later. Those messages in the console are just the AJAX requests sent to the API, it just comes from js9support.min.js because that's where JQuery got redefined.

Ignoring that worrying detail, I can't quite get my head around how the related sources datatable is configured or why it's not ordering properly. I can see a query parameter get changed to asc or desc when I click a column, but the data in the API response is in the same order.

@marxide
Copy link
Contributor

marxide commented Sep 24, 2020

Found the datatables problem! It's in the custom related action for SourceViewSet. It was doing pagination, but not filtering. I pushed the necessary change in bb4d4c8.

@ajstewart
Copy link
Contributor Author

Found the datatables problem! It's in the custom related action for SourceViewSet. It was doing pagination, but not filtering. I pushed the necessary change in bb4d4c8.

Great thank you! Did you happen to read something on that? Just wondered if I was looking in the wrong place in the docs for future reference.

@marxide
Copy link
Contributor

marxide commented Sep 25, 2020

Found the datatables problem! It's in the custom related action for SourceViewSet. It was doing pagination, but not filtering. I pushed the necessary change in bb4d4c8.

Great thank you! Did you happen to read something on that? Just wondered if I was looking in the wrong place in the docs for future reference.

It wasn't easy to find! ModelViewSet inherits GenericViewSet which inherits GenericAPIView. In the DRF docs for generic views, it mentions filter_queryset: https://www.django-rest-framework.org/api-guide/generic-views/#filter_querysetself-queryset

@srggrs
Copy link
Contributor

srggrs commented Oct 1, 2020

Need the CHANGELOG as per Slack conversation, then it's ready!

@ajstewart
Copy link
Contributor Author

Need the CHANGELOG as per Slack conversation, then it's ready!

What do you think of this changelog? https://github.com/askap-vast/vast-pipeline/blob/website-improvments/CHANGELOG.md

@srggrs
Copy link
Contributor

srggrs commented Oct 1, 2020

Need the CHANGELOG as per Slack conversation, then it's ready!

What do you think of this changelog? https://github.com/askap-vast/vast-pipeline/blob/website-improvments/CHANGELOG.md

I think is great! Thx! I wonder if need an extra # in every section to make everything much smaller and compacted...

Added intro and also links to comparisons.
@ajstewart
Copy link
Contributor Author

Need the CHANGELOG as per Slack conversation, then it's ready!

What do you think of this changelog? https://github.com/askap-vast/vast-pipeline/blob/website-improvments/CHANGELOG.md

I think is great! Thx! I wonder if need an extra # in every section to make everything much smaller and compacted...

I've updated it again, also included an intro and the links to the comparisons.

srggrs
srggrs previously approved these changes Oct 1, 2020
@ajstewart ajstewart dismissed stale reviews from srggrs via 28a9ee7 October 1, 2020 06:12
ajstewart and others added 2 commits October 1, 2020 16:12
@ajstewart ajstewart requested review from marxide and srggrs October 1, 2020 06:14
@ajstewart
Copy link
Contributor Author

Ok I'll wait for @marxide to comment on the previous comments made from the last review and then it should be ready.

@ajstewart
Copy link
Contributor Author

@srggrs, @marxide confirmed the last two review comments do not require any changes so it's at the same point you approved it earlier. So I'm going to merge this so @marxide can potentially work on the other branches with this in place now.

@ajstewart ajstewart merged commit 34223e3 into master Oct 1, 2020
@ajstewart ajstewart deleted the website-improvments branch October 1, 2020 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority An issue that needs to be prioritised.
Projects
None yet
3 participants