Don't display carousel tabs if there's no data to display #1169
Conversation
…f on tabs - Channel model: update the regex to allow for dash (it didn't but ETLs create them with dash and so you can't edit them) - Added a new tab button wrapper that loads data so it can tell if it needs to be displayed or not. This is WIP
We now need to load and wait for the component to show up before checking for carousel tabs for the featured carousel.
jkachel
force-pushed
the
jkachel/4528-resource-carousel-empty-tabs-fix
branch
from
6ebe8dd to
be0f09c
Compare
The one here will only ever take a FeaturedDataSource
jkachel
added
Needs Review
abeglova
left a comment
abeglova
left a comment
There was a problem hiding this comment.
LGTM. One small question
| label: React.ReactNode | ||
| cardProps?: CardProps | ||
| data: D | ||
| eager?: boolean |
There was a problem hiding this comment.
Looks like not currently used.
On production right now, the carousels only load the data once the tab is clicked into. With this PR, they will load all the tab data immediately (eagerly). So we'll get a bunch more API requests—for example, the homepage has 7 carousel tabs, so it will make 7 API requests on load. Similarly, the dashboard page has lots of carousels. James and I discussed the idea of making the eager behavior opt-in.
@jkachel You probably have a better sense than I do whether all those parallel API requests would be problematic (for the server). I'm happy to do always-eager and make opt-in later if seems appropriate.
There was a problem hiding this comment.
This is set up so only the 4 in the Featured tab eager load - the rest lazy load. So, you should only get 4 from the featured carousel plus single ones for the other carousel(s), and the featured ones shouldn't have to reload if you select tabs (since React Query should cache that response if it's successful).
FWIW, and the code sorta gets halfway there now, we can use a similar pattern to the DataPanel to eager load the tab buttons; if we do that, we can add the eager toggle there. (It seemed more like Featured was special in this regard so didn't go all the way with it here.)
(I was working on making this flaggable with that prop but I went a different route instead. That's a bit I missed from that so did in edef882 .)
abeglova
added
Waiting on author
and removed
Needs Review
| /** | ||
| * Tab button that loads the resource, so we can determine if it needs to be | ||
| * displayed or not. This shouldn't cause double-loading since React Query | ||
| * should only run the thing once - when you switch into the tab, the data | ||
| * should already be in the cache. | ||
| */ |
There was a problem hiding this comment.
I thought this was going to take more refactoring. Good idea 😁
jkachel
force-pushed
the
jkachel/4528-resource-carousel-empty-tabs-fix
branch
from
ae0c08b to
edef882
Compare
|
pre-commit.ci run |
|
pre-commit.ci run |
What are the relevant tickets?
Closes mitodl/hq#4528
Description (What does it do?)
Adds a new tab button component for the featured courses that can load the data that will be displayed in the tab, and then will disable itself if there's no content for the tab.
This also fixes a small issue with the regex validator for channels - they get created with names that have
-in them, but the validator didn't include that so you can't edit channels that were created via pipeline.How can this be tested?
Automated tests should pass.
Populate your featured lists with 0 items:
./manage.py populate_featured_lists 0You should see no tabs in Featured Courses.
Then, do the same thing but add some stuff:
./manage.py populate_featured_lists 10You should see the tabs appear now.
Additional Context
The tabs buttons do the loading now - so they appear as the data is retrieved. Locally, this means they pop in sorta slowly as for whatever reason these API hits take a while. Not sure if it would be better to have them load the tab, then remove themselves if they load 0 resources (it seems kinda disruptive either way).
This may highlight efficiency issues with the featured resources API - as noted, it's pretty slow for me but I'm not sure if that's the API being slow or if it's just Docker networking on macOS (which is pretty poky anyway).