-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Update PageControls buttons to use v5.3.2 syntax #8579
Update PageControls buttons to use v5.3.2 syntax #8579
Conversation
Run & review this pull request in StackBlitz Codeflow. |
Confirmed: Leilei332 has already signed the Contributor License Agreement (see contributing.md) |
Thanks @Leilei332 I think this is a good idea. |
IMO there are some buttons missing: |
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.
Thanks @Leilei332 these are worthwhile improvements. Just one small point below
I recognise that you disagree but we have to decide on a single approach and use it consistently. I don't think that the whitespace makes things more readable, rather the opposite: it means that the brain has to process the two parts separately, rather than seeing them as a single symbol. The other consideration is consistency with the way that we write widgets: we do not write A further but minor point is that omitting the whitespace will lead to smaller tiddlers. I will push an update to make things consistent on that basis. |
In the end it is your decision. So I'll change my code in pending PRs accordingly. |
It is no fun deciding these things, and I try to take other peoples views into account, but sometimes we're face with these binary choices where there is no answer that is going to please everyone. |
* Omit whitespace in conditional syntax * Define tf.get-tags function to avoid duplicated text substitution * Rewrite advanced search button
The code is much readable now! Thank you. |
Thanks @Leilei332 @pmario – is this ready to merge now? |
@Jermolene we should make a considered decision as to when we want to prefix the names of functions with I can understand the rationale behind doing so for global functions or those that can pollute the variable namespace for user content, however is it really necessary in the context of page control buttons? I find that the prefix reduces the legibility and expressiveness of the code. |
I did start a "discussion" some time ago and did reply there: #7955 (comment) |
Thanks @Leilei332 I am inclined to agree with @saqimtiaz that we should only use the |
Thank you @Leilei332 |
It is strange that PageControls buttons uses list widget to display text and icons.
This PR rewrites PageControls buttons and actions to use newer syntax. Including: