Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default to Open #1

Closed
m opened this issue Nov 13, 2015 · 0 comments
Closed

Default to Open #1

m opened this issue Nov 13, 2015 · 0 comments

Comments

@m
Copy link
Contributor

@m m commented Nov 13, 2015

The quality of our code and our product depend on the amount of feedback we get and on the amount of people who use them. If we’re developing behind closed doors, we are putting artificial limits to both.

We have done our best work in the open, let’s continue working this way.

@nb nb changed the title Placeholder issue Default to Open Nov 23, 2015
@mtias mtias added the OSS label Nov 23, 2015
artpi added a commit that referenced this issue Feb 8, 2016
…tions

Post-editor reduxification: add redux actions #1
@apeatling apeatling closed this Nov 13, 2017
ghost pushed a commit that referenced this issue Jun 14, 2018
Before this change, accessing https://wordpress.com/log-in would, most of the time, show a "WordPress.com" default title tag and no valid link rel=canonical (although the code has both a title - WordPress.com Log-In, and a rel=canonical defined). The reason is that the title and rel=canonical is only rendered properly when the page is NOT loaded from cache (keyword: SSR). In development, the first load would trigger the correct version of the page (observe the page title), while the 2nd one would make both the title and the rel="canonical" reset to the default values (or disappear).

After this change, the title and the rel=canonical should be rendered with each page refresh. What the commit includes:

1) Fixes the on-read cacheKey, making it use request.path instead of request.pathname (which is always undefined)
2) When dispatching ROUTE_SET (the Redux action that was actually clearing the title), it also informs the system that this is a SSR request. Therefore, the reducers can make decisions based on that.
3) Modifies the reducers that were setting the title and rel=canonical to blank/defaults such that they don't change the value when this is a first-time SSR rendering.

Rewinding to the problem, described in a few easy steps:

1. The state.documentHead (filtered nodes: themes, ui nodes) are loaded from cache (server/pages/index.js:200)
2. When doing the server-side rendering, ROUTE_SET is called ( server/isomorphic-routing/index.js, setRouteMiddleware) after the data at #1 was loaded from cache: `context.store.dispatch( setRouteAction( context.pathname, context.query, true ) );`
3. Because ROUTE_SET is called, the reducer cleans documentHead values ( client/state/document-head/reducer.js: 26, 35, 44, 53 ): `[ ROUTE_SET ]: () => '',`
4. .. and therefore the title, meta, and links are rendered with blank or default values on page refresh.

I thought of the following variants for fixing:
1. Remove ROUTE_SET 'reset to default' reducers for links, title, meta - too dangerous, too many uncontrolled side-effects, since we very probably expect the route change to clear this data
2. Let ROUTE_SET reducer know if this is a first-time SSR load and keep the previously loaded value (the one that the state has) - ADOPTED solution
3. Introduce a middleware that sets the links, title, meta after they were cleaned by ROUTE_SET. But since ROUTE_SET modifies the state, I would have needed to access the previous state OR to hold the values somewhere else in the context (context.cacheLoadedState), which would change too much of the flow in pages/index : getDefaultContext.
4. Introduce another action like ROUTE_SET_SSR, which would be called instead of ROUTE_SET. But practically that would mean that all the places where ROUTE_SET has reducers would need a ROUTE_SET_SSR that would replicate the behavior (except where we wouldn't want that to happen).
5. Do not call ROUTE_SET at all when SSR-ying, but that would mean that all the reducers where ROUTE_SET actually is useful would not be triggered.

How to (happy-)test:
1) Checkout the branch and start the server `npm start`. You can also do it on calypso.live, but locally you have more control on the cache.
2) Access https://calypso.localhost:3000/log-in
3) See the page title. Should contain 'Log In'. In productio probably it doesn't.
4) Look at the page source (not Inspector, but rather real page source that was initially generated). You should see a rel=canonical pointing to the /log-in source.
5) Access https://calypso.localhost:3000/log-in?redirect_to=https%3A%2F%2Fwordpress.com%2Fgo%2Ftutorials%2Fthree-tips-for-taking-a-vacation-thats-good-for-you-and-your-business%2F (so now we have a parameter) and repeat step 4. The canonical should be the same as before. Try a few more URLs that have redirect_to.
6) Access themes section and make sure everything is ok, as in production. Themes section is also SSR-ed and uses the cache. Watch for the page title, hoping that everything works as expected (or better than expected).
7) If you know what 'ui' is used for in the state, then also do a regression testing to see what is happening.
jakejones1984 added a commit that referenced this issue Jul 1, 2018
…nput’s date

Previously when blurring input #1 the value would be automatically be “copied” to input #2 if input #1’s value was the same as that stored in state. This is due to the way the DateUtils.addDayToRange() method works. Fix this by checking whether the value of the input that has just been blurred is the same as that stored in state. If so then don’t trigger an update of the selected date range. Adds test coverage.
jakejones1984 added a commit that referenced this issue Jul 31, 2018
…nput’s date

Previously when blurring input #1 the value would be automatically be “copied” to input #2 if input #1’s value was the same as that stored in state. This is due to the way the DateUtils.addDayToRange() method works. Fix this by checking whether the value of the input that has just been blurred is the same as that stored in state. If so then don’t trigger an update of the selected date range. Adds test coverage.
sirreal added a commit that referenced this issue Dec 4, 2018
Enforce always template curly spacing
sirreal added a commit that referenced this issue Dec 4, 2018
Enforce always template curly spacing
sirreal added a commit that referenced this issue Dec 5, 2018
Enforce always template curly spacing
sirreal added a commit that referenced this issue Dec 5, 2018
Enforce always template curly spacing
sirreal added a commit that referenced this issue Dec 5, 2018
Enforce always template curly spacing
sirreal added a commit that referenced this issue Dec 6, 2018
Enforce always template curly spacing
sirreal added a commit that referenced this issue Dec 11, 2018
Enforce always template curly spacing
getdave added a commit that referenced this issue Jan 7, 2019
…nput’s date

Previously when blurring input #1 the value would be automatically be “copied” to input #2 if input #1’s value was the same as that stored in state. This is due to the way the DateUtils.addDayToRange() method works. Fix this by checking whether the value of the input that has just been blurred is the same as that stored in state. If so then don’t trigger an update of the selected date range. Adds test coverage.
getdave added a commit that referenced this issue Jan 23, 2019
…nput’s date

Previously when blurring input #1 the value would be automatically be “copied” to input #2 if input #1’s value was the same as that stored in state. This is due to the way the DateUtils.addDayToRange() method works. Fix this by checking whether the value of the input that has just been blurred is the same as that stored in state. If so then don’t trigger an update of the selected date range. Adds test coverage.
getdave added a commit that referenced this issue Jan 25, 2019
…nput’s date

Previously when blurring input #1 the value would be automatically be “copied” to input #2 if input #1’s value was the same as that stored in state. This is due to the way the DateUtils.addDayToRange() method works. Fix this by checking whether the value of the input that has just been blurred is the same as that stored in state. If so then don’t trigger an update of the selected date range. Adds test coverage.
jeffersonrabb added a commit that referenced this issue Jan 27, 2019
…Media Modal-provided size data, and persist as an attribute.
getdave added a commit that referenced this issue Feb 1, 2019
…nput’s date

Previously when blurring input #1 the value would be automatically be “copied” to input #2 if input #1’s value was the same as that stored in state. This is due to the way the DateUtils.addDayToRange() method works. Fix this by checking whether the value of the input that has just been blurred is the same as that stored in state. If so then don’t trigger an update of the selected date range. Adds test coverage.
sgomes added a commit that referenced this issue Jan 29, 2020
Initial implementation
nsakaimbo added a commit that referenced this issue Feb 5, 2020
sgomes added a commit that referenced this issue Feb 18, 2020
Initial implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants