/TagFront - Tag fronts on DCR! - #7752
Merged
Merged
Conversation
alinaboghiu
reviewed
May 17, 2023
| getArticleSchema, | ||
| getFrontSchema, | ||
| getNewsletterPageSchema, | ||
| getIndexPageSchema, |
Contributor
There was a problem hiding this comment.
being pedantic here but index is quite an overloaded term, maybe we could go with tagPage instead? I think this is what we call them in the business no?
Contributor
There was a problem hiding this comment.
actually, probably better to call it a TagFront, which it is, then we'd also avoid the TagPagePage business
Contributor
Author
There was a problem hiding this comment.
Your definitely right, in the spirit of hacking speed I'm going to keep going, but will be refactoring & tidying all this code on Friday
Update FrontPagination.tsx
|
Size Change: -10.4 kB (-2%) Total Size: 571 kB
ℹ️ View Unchanged
|
mxdvl
reviewed
Jun 22, 2023
Contributor
There was a problem hiding this comment.
Impressive!
Grouping with JS Map is very powerful and may simplify some of your grouping logic
EDIT: There’s an upcoming Array.prototype.group feature that will make grouping easier!
OllysCoding
commented
Jun 22, 2023
AshCorr
reviewed
Jun 22, 2023
| return fallback; | ||
| }; | ||
|
|
||
| export const Sources = ({ sources }: { sources: ImageSource[] }) => { |
Member
There was a problem hiding this comment.
Not in the scope of this PR, but we could really do with a "generic" picture element 😅
Contributor
Author
There was a problem hiding this comment.
I was definitely thinking of this, I think it should be pretty easy to put together!
| children: Array.from(node.childNodes).map(buildElementTree), | ||
| }); | ||
| default: | ||
| return jsx(node.tagName.toLowerCase(), { |
Contributor
There was a problem hiding this comment.
In order to use this we want to be pretty sure that we won't receive a tag that doesn't accept children. I.e none of these elements https://developer.mozilla.org/en-US/docs/Glossary/Void_element Otherwise this will break the page. So it could be safer to handle tags explicitly and return null if an unexpected tag is received.
Contributor
Author
There was a problem hiding this comment.
Thanks - I'm pretty sure we wont see any of these other elements, but I'll take a look at raising another PR in the future to refactor all this code to be more heavily re-used & avoid any potential errors like this!
mxdvl
reviewed
Jun 23, 2023
mxdvl
approved these changes
Jun 23, 2023
mxdvl
reviewed
Jun 23, 2023
mxdvl
reviewed
Jun 23, 2023
mxdvl
reviewed
Jun 23, 2023
Comment on lines
+141
to
+143
| // If there is a gap of more than 1 number between this entry and the last one | ||
| const shouldPrefixDots = | ||
| page > 1 && pageRange[index - 1] !== page - 1; |
Contributor
There was a problem hiding this comment.
Not sure if this is helpful in this case, but distance between two numbers can also be obtained by calculating the absolute value of the difference between them: Math.abs(page - pageRange[index - 1]) > 1
Contributor
Author
There was a problem hiding this comment.
In this case I don't think it does make a difference, but definitely helpful to know!
alinaboghiu
approved these changes
Jun 23, 2023
Contributor
|
There are some visual small changes to the titles of tone title–has a line to the right–and the container titles–larger and bolder font.
|
mxdvl
reviewed
Jul 5, 2023
Comment on lines
+169
to
+172
| trailsByDay.map(({ day, trails: dayTrails }) => ({ | ||
| day, | ||
| trails: dayTrails, | ||
| })), |
Contributor
There was a problem hiding this comment.
I think this map does nothing 😉
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds support for tag fronts in DCR!
Completed features in this PR
groupTrailsByDates.ts- Logic for grouping trailsTagFrontLayout.tsx- Full working layout for tag frontsFrontPagination.tsx- Pagination logic that allows users to navigate between pages on tag frontsTagFrontHeader.tsx- Header for tag fronts which can include a title, description & image if they're provided for the tagFuture work required
Screenshots
tone/minutebyminute
profile/josh-haliday
