fix: resolve publication manager issue "missing index.html" (#1689)#19
fix: resolve publication manager issue "missing index.html" (#1689)#19Zayed-Ahmed-dev wants to merge 1 commit into
Conversation
lexoyo
changed the title
lexoyo
left a comment
lexoyo
left a comment
There was a problem hiding this comment.
Hey Zayed! Thanks for the PR, it fixes #1689 nicely 👍
I did a code review, hope you find it interesting, there is no major problem at all. Just an edge case to handle when there is a data source
Other suggestions only if you feel like it:
- You could factor the
hasIndexcheck into a function as it is currently done in 2 places (publish()andgetHtmlFilesYield()) - We could use a small unit test but we can add it later maybe in another PR?
Thank you for this, and it's great to have you around !
| const name = page?.name || '' | ||
| return name.toLowerCase() === 'index' | ||
| }) | ||
| console.log(hasIndex); |
| const siteSettings = { ...this.editor.getModel().get('settings') as WebsiteSettings } | ||
| const pages = (projectData.pages as any[]) || [] | ||
| const hasIndex = pages.some(page => { | ||
| const name = page?.name || '' |
| message: | ||
| 'No page named "index" found. The first page will be used as homepage. Rename a page to "index" to control homepage.', | ||
| }) | ||
| } |
| const pages = (projectData.pages as any[]) || [] | ||
| const hasIndex = pages.some(page => { | ||
| const name = page?.name || '' | ||
| return name.toLowerCase() === 'index' |
There was a problem hiding this comment.
The same check is done again below in getHtmlFilesYield(), but written differently — that one uses getPageSlug(name). They can disagree on names like "Index Page" or " index " with spaces. Simpler and safer to use the same line in both places: getPageSlug(name) === 'index'.
| const slug = getPageSlug(page.get('name')) | ||
| let slug = getPageSlug(page.get('name')); | ||
| // if no index page exist make the first page as home page | ||
| const firstPageId = pages[0]?.getId() |
There was a problem hiding this comment.
pls move this line above the loop as it doesn't change between iterations
| type: 'warning', | ||
| group: 'publication', | ||
| message: | ||
| 'No page named "index" found. The first page will be used as homepage. Rename a page to "index" to control homepage.', |
There was a problem hiding this comment.
Nice to have / just an idea: maybe we could add which page will become the homepage, pages[0]?.name is available
There was a problem hiding this comment.
or even better, more like other messages in silex:
`Page "${pages[0]?.name}": Will be used as homepage because no page is named "index". The homepage is the page served at the root URL of your site.`
| // Get the data to publish, clone the objects because plugins can change it | ||
| const projectData = { ...this.editor.getProjectData() as WebsiteData } | ||
| const siteSettings = { ...this.editor.getModel().get('settings') as WebsiteSettings } | ||
| const pages = (projectData.pages as any[]) || [] |
There was a problem hiding this comment.
pages could/should be a Page[] right?
you probably want
const pages = projectData.pages ?? [] as Page[]
|
|
||
| // Transform the file paths | ||
| const slug = getPageSlug(page.get('name')) | ||
| let slug = getPageSlug(page.get('name')); |
There was a problem hiding this comment.
no semicolons in this project — drop the ; at the end
| return name.toLowerCase() === 'index' | ||
| }) | ||
| console.log(hasIndex); | ||
| if (!hasIndex) { |
There was a problem hiding this comment.
when the site has a data source, the user probably use custom permalinks per page, so the fix isn't needed and breaks publication in some cases
please skip both the warning and the fix when a data source is configured. You can use enable11ty() from cms/publication.ts to know if there is a data source
| // if no index page exist make the first page as home page | ||
| const firstPageId = pages[0]?.getId() | ||
|
|
||
| if (!hasIndex && page.getId() === firstPageId) { |
There was a problem hiding this comment.
here too, skip the fix when a data source is configured
|
Hi @Zayed-Ahmed-dev, just a friendly check-in on this PR. It has been quiet for about 3 weeks since the last review round, and we want to be respectful of your time: are you still planning to work on the requested changes, or have your priorities changed? Either answer is totally fine. If you have time and want to continue, the review comments are ready to address. If not, just let us know and we can close the PR or pick it up later on our side. Thanks again for the work you already put into this. |
2 participants
Description
This PR addresses the issue reported in silexlabs/Silex#1689.
Currently, publishing fails and throws an error when there is no first page defined in the website.This PR improves the behavior by:
indexThis provides a smoother publishing experience and prevents the publication process from failing unexpectedly.
Changes made
Before
silexbefore.mp4
After
silexAfter.mp4
Related Issue
Related to silexlabs/Silex#1689