Hid applications that are deprecated by default #78
Conversation
|
Hello @wlach! This PR is ready for review. |
src/pages/AppList.svelte
Outdated
| @@ -1,4 +1,7 @@ | |||
| <script> | |||
| import { fade } from 'svelte/transition'; | |||
| let visible = true; | |||
There was a problem hiding this comment.
We should use a more descriptive variable name here.
| let visible = true; | |
| let showDeprecated = true; |
src/pages/AppList.svelte
Outdated
| @@ -1,4 +1,7 @@ | |||
| <script> | |||
| import { fade } from 'svelte/transition'; | |||
There was a problem hiding this comment.
I appreciate the enthusiasm and desire to experiment, but let's leave the animated transitions to a seperate PR / issue. :) This helps describe why: https://homes.cs.washington.edu/~mernst/advice/version-control.html#logical-unit
For now I'd like to mostly concentrate on making sure that all the required UI elements are there, rather than on how they look.
There was a problem hiding this comment.
I thought it would make the boring UI looks nicer 😅
Will be careful about it in future.
|
This is the updated UI.
|
mhmohona
changed the title
src/pages/AppList.svelte
Outdated
| <a href="/apps/{app.name}">{app.name}</a> | ||
| {#if app.description}<i>{app.description}</i>{/if} | ||
| </p> | ||
| {#if !app.deprecated} |
There was a problem hiding this comment.
You can skip the if/else (and the duplicated logic for display which violates the DRY principle) by doing the following on display:
| {#if !app.deprecated} | |
| {#if showDeprecated || !app.deprecated} |
Alternately, we could probably filter the applications in the filterApps function above. I'd accept either solution for now.
There was a problem hiding this comment.
Edited as per your suggestion @wlach . 😄
There was a problem hiding this comment.
Hi @mhmohona -- it looks like the logic is still duplicated unfortunately. To be clear, you should only one block that iterates through the app list and no higher level if/else (i.e. the logic should be substantially the same as before, just with that extra piece of logic that I showed above).
There was a problem hiding this comment.
Sorry sorry @wlach, at that time I was confused. 😅
I hope now I could do what you asked.
I did not know about DRY principle before. I really appriciate the way you are helping to learn, to improve.
This PR is submitted for issue #53.
Here I have added a checkbox and if the check box is unchecked, deprecated apps will be hidden. If it is checked, all applications will appear.