codethesaur.us
codethesaur.us copied to clipboard
Issue 480 - alert for unavailable combinations
What GitHub issue does this PR apply to?
Resolves #480
What changed and why?
Changes:
-
Added a static JavaScript file that, every time a new language or concept is chosen, makes a fetch API request in order to check if the respective structure is available or not. If the structure is not available, the go button will be disabled and an alert will become visible in the respective card (much like the "Oops" page itself) with the following information:
- explanation that the language/concept combination is not available
- GitHub links for creating the language/concept
- template that the user can use to create the language/concept via the above GitHub links
-
Functions in the js file were added to disable the ability to have duplicates in the "compare concepts" card.
-
Added the alert div to the reference card and compare card in index.html
-
A scripts block was added to base.html and index.html so the script loads only on index.html.
Reasons:
- The reason for implementing this feature is to alert users which combinations are available without having to leave the home page. This will give the users a better experience as opposed to constantly switching back and forth from "Oops" pages and the home page.
Checklist
- [X] I claimed any associated issue(s) and they are not someone else's
- [X] I have looked at documentation to ensure I made any revisions correctly
- [X] I tested my changes locally to ensure they work
- [ ] (If editing Django) I have added or edited any appropriate unit tests for my changes
Any additional comments or things to be aware of while reviewing?
Because the structures for Ada (version 12) Classes and Bash (version 5) Classes are not available, the alert will appear immediately every time a user visits the home page. So, while the js script is working properly, it may be seen as unappealing to see on every visit, at least until the two structures are created.
Oooh, I like where this is going! I have a few thoughts/questions, let me get through some work things first and I'll come back to review this better.
Oooh, I like where this is going! I have a few thoughts/questions, let me get through some work things first and I'll come back to review this better.
No problem!
I did look at this, wrote out comments, apparently didn't send, then lost the window. Oops 😬
Anyway, I like this as a starter. The first thing I noticed is that only one of the languages was disabled. I thought it was because that was the only one unavailable but realized you disabled the other language in the drop down. This makes sense, but also felt a bit odd considering the other lang-concepts that aren't available are still selectable.
Second, the box telling you the lang-concepts aren't there is kind of large for what it's trying to say. I wonder if you could just have 1-2 lines that say "X in Y doesn't exist. Click here to learn about contributing." and then if you click that it takes you to docs, makes the box bigger (like it presently is), or a modal (though I don't like this as much)?
Something like:
Third, I noticed it's API call is basically to just grab the lookup page. While that can work, it does a lot of rendering that doesn't need to happen, but also would mess up the stats counter attached to those pages. I'd rather just expose a few new API endpoints of languages, concepts, and/or languages and concepts. If you don't know how to do that in Python/Django, I can probably try to work on making those so you can use those instead.
What do you think?
The reason I only disabled the one counterpart language in the dropdown is just to prevent duplicates. I see what you mean though, it may make more sense to just disable all the unavailable language/concept combos. Going this route, a couple of changes will need to be made.
First, because all the unavailable languages will be disabled, the individual alert messages will not be seen. Instead of those, we could maybe have a general message displayed at the top or bottom of the page. As you suggested, we could keep it short, something like "If there is a language/concept combination you want that is not available, learn how to help out!", and then put in a link to the docs.
Example:
Second, the lang/concept combinations would have to be checked from the initial rendering of the page, and I think this could be done without API calls. I can try doing this by adding a third attribute to the meta_data_langs dict that is passed to index.html, something like an array of the concepts that are present in each lang/ver directory. Then, in the index.html, the available concepts can be added as classes (or a different html attribute). We can use static js to add event listeners, similar to what I did before, that will dynamically enable/disable concept and language options based on the current options' class list.
Example:
On the other hand, I think using API calls to new API endpoints can also work as long as they can detect what lang/concept combos are available. This could be executed similarly to the above method where the static js makes the API calls initially and then sets event listeners to enable/disable the options based on the fetched data. If we'd prefer this method, then I could try my hand at making the API endpoints first and then ask you for help if I'm having trouble putting things together.
Overall, I could try executing this without the API calls first, and then, if it doesn't pan out, could try setting up the API endpoints. However, I am open to doing whatever you think would be best! Thoughts?
Oops, I guess I never got back to you! Sorry about that.
I say go ahead if you want. If you want to try the non-API way, then that's fine with me. If it doesn't work or whatever, switch or we can talk about it or whatever.
And I do like that blue box at the bottom better than the prior way!
Hey @TomReilly1, I figured I'd check up on this again. How's it going? Is there anything you need from me to help get it out the door?
Hi @TomReilly1 , I'm doing another checkin. How is this going? Do you need anything from me?
Hey @geekygirlsarah, sorry about the delay! I haven't been able to work on it recently, but I am going to work on it and aim to have it done by the end of this week!
That works! No rush, just mostly wanted to know an update. Thanks!
Hey @geekygirlsarah, I finished up my code for this issue a couple of days ago, so it should be good to go! Let me know if there are any further problems or if you have any questions or concerns.
I get notices of all issue and pull request activity, and I try to get around to them in the order they came in. I've been out of town with limited computer access, so I appreciate your patience as I catch up.
Alright, I'm liking this more! Though in testing it, I'm finding it's hard to choose a concept first then pick the languages* because some are disabled by default. I can't choose "functions" if they do exist in two languages* because the languages* aren't chosen first.
This might be a case of maybe all the concepts should be enabled and if it's not in that language*, then disable the languages* it's not in. Otherwise the UX is pretty terrible.
I see what you mean. It did feel a little wonky trying to find a lang/ver combo. Maybe what we could do is, first (as you suggested) enable all concept options. Then when a concept option is selected, it can check to see if the lang(s) are available. If not it can automatically switch to a language that is available. That way the user can freely select the concepts, but then have the langs/vers disabled accordingly.
I asked a few people at my local tech community "build night" what they thought of it, and I think we've all kind of liked it, but it feels like a tiny box in this huge space. Could you just widen it and make it fix the width of that text on one line? Maybe decrease its padding around it a little too?
I think changing the width to 'fit-content' would be a good solution. Also, I removed the top-margin from the alert container and removed the margin from the alert itself. So, now it looks like the picture below and utilizes the space bit better. How do feel about this?
Maybe what we could do is, first (as you suggested) enable all concept options. Then when a concept option is selected, it can check to see if the lang(s) are available. If not it can automatically switch to a language that is available. That way the user can freely select the concepts, but then have the langs/vers disabled accordingly.
[image]
Yeah, that looks good! Ship it! :shipit:
I think changing the width to 'fit-content' would be a good solution. Also, I removed the top-margin from the alert container and removed the margin from the alert itself. So, now it looks like the picture below and utilizes the space bit better. How do feel about this?
[image]
Also great! Double-ship! :shipit: