-
Notifications
You must be signed in to change notification settings - Fork 197
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
The Vaadin Way of building the Presentation Layer #3516
base: latest
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good already 👍 No complaints about the content, good and informative points that I found well explained. Added some comments about some minor tweaks that I might like to see (typos, sentence flow improvements, things that took a moment of puzzling out that could perhaps be avoided with minor adjustments, and my preference for Oxford commas which is completely optional but I just personally find it more readable).
|
||
image:images/main-layout.png[width=800] | ||
|
||
It consists of a header and a content area. The content are contains whatever the current view is. The header consists of three different components: the application's logo, navigation links to the _Employees_, _Teams_ and _Locations_ views, and the current user's avatar. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The content are contains whatever the current view is.
are -> area
|
||
### Team Selection Panel | ||
|
||
The master-part of the master-detail is the team selection panel: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think master-part
would read better without the dash. Same goes for detail-part
later.
|
||
image:images/view-master.png[width=200] | ||
|
||
At the top, there is a header containing the name of the Teams view, a button for creating new teams and a text field for filtering the list of teams. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many views on this matter, but personally I prefer Oxford comma -- i.e. a comma before each 'and' in lists of three or more items. I find lists easier to read with the Oxford comma, especially when each item contains a longer bit of text, like this one does. But of course it's also important to be consistent, so if the comma is added here, it should be added to all the other lists as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer Oxford comma in our guidelines: https://vaadin.com/docs/latest/contributing/docs/styleguide#lists
And we have a Vale rule to check for that as well: https://github.com/vaadin/docs/blob/latest/.github/styles/Vaadin/OxfordComma.yml
|
||
To the left, there is a sidebar with two sections: one with general information about the team and another with a list of managers of the team. From the mock-up it is unclear whether the managers are clickable or not. It is also unclear whether the sidebar is resizable or not, although the lack of a splitter indicates it has a fixed width. Again, these are things you should find out before you start implementing, as it affects which components you can use. | ||
|
||
To the right, there are tabs that control the contents of the rest of the panel. This indicates that the entire team details panel is in fact a nested layout with three sub-views: _Employees_, _Salaries_, and _Documents_. In fact, the team details panel looks like this: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two 'in fact' in close succession. I might switch the second one to 'In other words', or even tweak the final sentence all the way to In other words, the team details panel should look like this:
|
||
The second generic component is the item with icon: | ||
|
||
image:images/items.png[width=300] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be better to have these items as separate files rather than all in one, or to otherwise separate them visually somehow (a grey background behind white items, perhaps)? It took me a moment to figure out that this wasn't the entire general information item set from underneath the Summary header, but three separate items that were just shown together for comparison.
|
||
You are now going to learn how to design the URL of the mock-up based on how a user is intended to navigate the view. The main focus is on deciding what _view state_ to store in the URL as path or query parameters. Now why is this important? | ||
|
||
Say you are working with the application, and want one of your colleagues to have a look at something. Now could ask them to open the application and tell them how to look up the information you want to show. However, if the relevant state was stored in the URL, you could send it to your colleague. The colleague would then only need to click the link to end up with the same view as you (after authenticating, of course). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now could ask them to open the application
'You could', or 'Now, you could' (possibly works without a comma too)
you could send it to your colleague
I might go with 'send that' instead
|
||
image:images/employees-sub-view.png[width=600] | ||
|
||
As with ordinary views, it is a good idea to look for common design patterns. In this case, you have another Master-Detail. At the top, there is a grid of team members. When you select a team member, its details show up in a bottom panel. The splitter between the grid and the bottom panel indicates the bottom panel is resizable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional, that the team member selection doesn't show in the grid? Took me a moment to find Cody Fisher in the list.
|
||
When a user uses a web application, it is the application itself that handles the navigation in respond to the user's actions. For example, in the mock-up application, the user would not modify the URL itself to select a team or a tab. Rather, the user would click on the team and the tab and the application would be responsible for updating the URL. | ||
|
||
When a web application updates the URL, it can do it in wo ways: either by pushing new entries to the history stack, or by replacing the current entry in the history stack. This affects the behavior of the browser's back button. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can do it in wo ways
wo -> two
|
||
If the user clicked the back button now, they would end up with a sorted employee grid without a selection. Another click on the button would deselect the team and show the search results. | ||
|
||
Finally, as with all rules of thumb, there may be exceptions. The important thing is to take back button behavior into consideration when designing the view URL and make sure it makes sense. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The important thing is to take back button behavior into consideration
I might tweak that to 'take the back button behavior', because when I was reading it for the first time, my brain first interpreted it as it being important to 'take back' something called 'button behavior' and then the sentence completely fell apart. It took a moment to rearrange that mentally into only 'taking' that 'back button behavior' into consideration.
…iew-structure/composition.adoc
…iew-structure/url-design.adoc
I checked the text as it is now. Please let me know when this PR is out of Draft mode by marking it Language Unchecked again so I check any changes and do another round of edits. Thanks. |
|
||
This consists of a header and a content area. The content area contains whatever is the current view. The header consists of three different components: the application's logo; navigation links to the _Employees_, _Teams_ and _Locations_ views; and the current user's avatar. | ||
|
||
The main layout is a UI component you have to implement. The header is so simple that it can be nested inside the layout. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to say here that, "the header is simple enough that it can be included in the main layout without having to create a separate component for it" ?
If so, then I would write something similar to above.
If it is something else then the text does not adequately convey that.
|
||
If the user now clicked the back button, they would be confused. It would be an even worse user experience. | ||
|
||
A good general practice is to push new entries to the history stack whenever the path of the URL changes, and replace the current entry whenever query parameters change. With this behavior, the use case described earlier would result in the following history stack, sorted from oldest to newest: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is another scenario where the user searches for different things, and the replacing the history stack results in retaining only the last search.
This is something for the product to decice on. So we should present both the pros and cons of this.
#3372 should be reviewed and merged before this PR!