Closed m1kola closed 1 year ago
I manually walked the netlify presentation versus the existing docs, and the only question I have is: do we /have/ to have the comment at the bottom of each page that identifies the PR merge which last modified that page? That's the only real difference between the existing and proposed, and it might be a product of netlify, but I'm not sure.
@anik120 I'm a big fan of small PRs and I tried to break it down (see #282 and #283 for example). However I beleive we can not break it down any further into smaller PRs which are mergable into master: too many things depend on each other. E.g.:
We can probably create a new base branch for these changes and split this PR into smaller PRs which won't be buildable until we merge all of them.
While working on this I treid to create commits which contain smaller pieces of work. Could you please try to review commit by commit instead of full PR diff? I'm sorry that I didn't suggest it earlier. If reviewing commit by commit is still not manageble - please let me know and we will discuss options.
and the only question I have is: do we /have/ to have the comment at the bottom of each page that identifies the PR merge which last modified that page? That's the only real difference between the existing and proposed, and it might be a product of netlify, but I'm not sure.
@grokspawn hmm, good catch! However I'm looking at the live version and I see that on some pages (1, 2, for example) have "Last modified" and some don't (e.g. this one).
I think we should be constent and either show it everywhere or hide it everywhere. What do you think?
Rebased on top of master. Please take another look.
I think we should be constent and either show it everywhere or hide it everywhere. What do you think?
My preference would be to not show it at all, but I don't think either outcome should block this.
I created a separate PR to remove "last modified" at the end of the page globally here: https://github.com/operator-framework/olm-docs/pull/289
This PR will need a rebase once we merge #289.
Rebased on top of master to fix conflicts with #289
@anik120 did you have any specific concerns, or was the 'change requested' just about the PR size?
/lgtm /approve
Prerequisites:
This PR:
Next steps (follow up PRs):