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

Refactor test console ahead of upcoming auth changes #6

Merged
merged 5 commits into from
Jul 15, 2024
Merged

Conversation

efixler
Copy link

@efixler efixler commented Jul 12, 2024

This PR makes some organizational changes to the test console preceding disabling open access to the test console via a short-lived token and requiring users to enter a token. No functional changes in this release, just tweaking the html/js/css.

Main changes here:

  • Remove the iframe that was previously the response target and replace it with a div
  • Update the HTML/CSS with a more coherent box model
  • Incorporate AlpineJS and bind the form submission to the Alpine lifecycle (and evaluate and learn Alpine)

Steps to Test

Again, no functional changes in this release, so it should operate exactly as https://stage.scrape.mozsoc-ml.nonprod.dataservices.mozgcp.net with some extremely minor visual changes.

Building this PR

  • Check out the branch

Without Go on your machine

  • In the branch root:
    • make docker-build
    • make docker-run

With Go 1.22.x on your machine

  • In the branch root:
    • make
    • ./build/scrape-server

Testing this PR

  • Point your browser to http://127.0.0.1:8080
  • Enter a url in the Enter a URL text field (like https://arstechnica.com/gadgets/2024/07/arduinos-plug-and-make-kit-lets-your-hacking-imagination-run-wild-sans-solder/)
  • Click "Hit It"
  • You should see metadata from the web page in the big text area
  • Using the same URL, try the Headless and Feed options from the pulldown. You should see errors in the big text area.
  • (For Feed, if you enter a valid RSS feed you should get results. Headless will work on mozgcp.net, but not in the local build you just made)

References and Decisions

  • In general, keeping the html simple and self-contained here is more important than download size/performance at this time. Hence everything is inline right now.
  • For more info about AlpineJS see here. Moving forward, this view will get more interactive, so I'm evaluating Alpine as a lightweight tool to use for these cases.

@efixler
Copy link
Author

efixler commented Jul 12, 2024

Comment from maxx (autocomplete not working) copied from Slack, will answer inline:

Does this need to be fully-responsive?

I'm not exactly sure what is precisely meant by the question, but I think I'd say it should be "pretty responsive", by which I mean that I expect it to be used generally at desktop dimensions and it should reasonably adjust for those. In particular, the metadata can be big so I want to make sure that we use the max available space for that and let the user adjust their browser if they want the text display to be wider or narrower.

It's certainly not going to win any beauty contests right now and it doesn't need to. If you have suggestions for how I can better handle resizing [within the constraint that this is intended to be very simple and as close to depedency-free as I can reasonably make it], I'd love suggestions.

Are there future UI bits going into it?

The next part is just to let the user add their own token, which I'll probably make hideable and storable if hey want it to be, just to reduce hassle (my #1 goal here is that it should be easy to get to to test and verify url output). No concrete plans beyond that.

Is there any other tool this sits next to that should "match" styles?

Nope :)

Copy link

@maxxcrawford maxxcrawford left a comment

Choose a reason for hiding this comment

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

I had a few suggestions and questions, but overall, this is great as is. Thanks!

<label for="url">Enter a URL:</label>
<input type="submit" value="Hit It">
<input type="url" name="url" id="url" value="https://" size="96" maxlength="200" pattern="https?://.*" required title="URL">
<select title="URL Type" id="actionSelect" onchange="updateFormAction()">
<select title="URL Type" x-model="formAction">

Choose a reason for hiding this comment

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

UX Suggestion: Swap the placement of the <input type="submit"> and <select> tag.

Having the submit button before the text input and URL type is a bit of an anti-pattern.

image

Copy link
Author

Choose a reason for hiding this comment

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

I'm glad the above worked with the trailing paren!

Copy link
Author

Choose a reason for hiding this comment

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

And great suggestion!

Copy link
Author

@efixler efixler Jul 13, 2024

Choose a reason for hiding this comment

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

Made this change in 7362a59, I think it's tight!

headers.append('Authorization', `Bearer ${token}`);
}
const formData = new FormData(form);
formData.delete('token');

Choose a reason for hiding this comment

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

Question: Why does the token deleted from the formData here?

Copy link
Author

@efixler efixler Jul 12, 2024

Choose a reason for hiding this comment

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

Only because we don't need to submit it with the form, it needs to go as a header. In the next PR I intend to pull that out of the form entirely.

Comment on lines 26 to 35
.response-container {
width: 100%;
min-width: 768px;
flex: 1;
overflow: auto;
border: 0.1rem inset #ccc;
padding-block: 0.2rem;
padding-inline: 0.4rem;
box-sizing: border-box;
}

Choose a reason for hiding this comment

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

Question/suggestion: Will users need to copy result contents?

If so, we might add user-select: all or another method to make the text inside more easily selectable for copy/pasting.

Suggested change
.response-container {
width: 100%;
min-width: 768px;
flex: 1;
overflow: auto;
border: 0.1rem inset #ccc;
padding-block: 0.2rem;
padding-inline: 0.4rem;
box-sizing: border-box;
}
.response-container {
width: 100%;
min-width: 768px;
flex: 1;
overflow: auto;
border: 0.1rem inset #ccc;
padding-block: 0.2rem;
padding-inline: 0.4rem;
box-sizing: border-box;
user-select: all
}

Copy link
Author

Choose a reason for hiding this comment

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

There's no use case for that right now, it is a good idea tho. At some point (probably not in the near future due to time constraints) I hope to break this down into a different sort of display

Copy link
Author

Choose a reason for hiding this comment

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

I tried this out and decided that it's better to let the user copy text at-will from the box (like if you want an image url). I did make the param explicit with text.

pre {
white-space: pre-wrap;
width: 100%;
}
label {
display: block;
font: 1rem 'Verdana'

Choose a reason for hiding this comment

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

(nit) Suggestion: Add multiple font families for better font choice fallback.

font-family: Verdana, Arial, Helvetica, sans-serif;

Copy link
Author

Choose a reason for hiding this comment

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

Updated in 7362a59

internal/server/templates/index.html Outdated Show resolved Hide resolved
@efixler
Copy link
Author

efixler commented Jul 15, 2024

Does this need to be fully-responsive?

Preview: the page's responsiveness overall, and particularly the url entry form, are much better in #7

Copy link

@maxxcrawford maxxcrawford left a comment

Choose a reason for hiding this comment

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

Runs great! Thanks for answering my questions as well. 🚀

@efixler efixler merged commit 0e48c95 into main Jul 15, 2024
1 check passed
efixler added a commit that referenced this pull request Jul 16, 2024
* Refactor test console ahead of upcoming auth changes (#6)
* Add token entry/storage functionality to test console (#7)
@efixler efixler deleted the token-on-index branch August 13, 2024 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants