Issue/2718 site search selection #2815
Conversation
My personal experience with the site picker seems to suggest the option 1. When I searched for a site would have been nice to have it available in the list right after. |
|
The code looks OK, and can be merged if we go with the option 2. |
|
Ready for review. Now when a hidden site is selected, it becomes visible. |
| @@ -446,6 +438,15 @@ public static Blog setCurrentBlog(int id) { | |||
| return currentBlog; | |||
| } | |||
|
|
|||
| public static Blog setCurrentBlogAndSetVisible(int id) { | |||
| Blog blog = getBlog(id); | |||
There was a problem hiding this comment.
In the current version of the code setCurrentBlogAndSetVisible and setCurrentBlog despite doing almost the same thing they have different code.
I also noticed that setCurrentBlog is very similar to getBlog.
My .2$: It's probably better to have setCurrentBlogAndSetVisible calling setCurrentBlog, and in order, change setCurrentBlog that should call getBlog.
There was a problem hiding this comment.
I also noticed that setCurrentBlog is very similar to getBlog.
Good catch! Didn't notice that. In fact, setBlog() returning a value didn't make sense so I looked at its usages and no one used the return value so I removed it.
It's probably better to have setCurrentBlogAndSetVisible calling setCurrentBlog and in order, change setCurrentBlog that should call getBlog.
So it was calling it before, but as the return value. Now, it calls it first and then sets the blog as visible. I believe that's what you meant.
@daniloercoli (adding your name just in case it doesn't ping you)
…for setCurrentBlogAndSetVisible
|
|
Issue/2718 site search selection
Two things:
getCurrentBlog()currently checks for blog visibility. With the site picker search, the user can select a non-visible blog. Clicking 'View Site' does not work for non-visible blogs. So there are two options:My vote would be 2. Visibility seems to be a purely UI-driven concept, and auditing the calls to it doesn't yield any concerns. However, @maxme suggested 1. Thoughts?
@roundhill @nbradbury @tonyr59h @aforcier