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

Enable priority checkbox for admin and participant #1562

Conversation

chena11356
Copy link

No description provided.

@@ -1 +1,3 @@
*~
.history/
math/.cpcache/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been using separate .gitignore files in each of the subdirectories, so I think we should stick with that pattern unless we have a broader conversation about switching to a single/main .gitignore.

@@ -1 +1,3 @@
*~
.history/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is .history coming from? I think this can stay if it's something we expect lots of other devs to have. But if it's a detail of your particular dev setup, I think it makes sense for you to just put that in your ~/.gitignore. If it stays though, it should have a comment about what it's for.

Comment on lines 88 to 96
from {
background-color: rgba(0, 0, 0, 0);
}
10% {
background-color: rgb(202, 227, 255);
}
to {
background-color: rgba(0, 0, 0, 0);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually like the compact view in cases like this.

Comment on lines 189 to 200
#agreeButton, #disagreeButton, #passButton {
#agreeButton,
#disagreeButton,
#passButton {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, prefer the more compact form here.

Comment on lines 359 to 380
display: -webkit-box; /* OLD - iOS 6-, Safari 3.1-6 */
display: -moz-box; /* OLD - Firefox 19- (buggy but mostly works) */
display: -ms-flexbox; /* TWEENER - IE 10 */
display: -webkit-flex; /* NEW - Chrome */
display: flex; /* NEW, Spec - Opera 12.1, Firefox 20+ */
display: -webkit-box; /* OLD - iOS 6-, Safari 3.1-6 */
display: -moz-box; /* OLD - Firefox 19- (buggy but mostly works) */
display: -ms-flexbox; /* TWEENER - IE 10 */
display: -webkit-flex; /* NEW - Chrome */
display: flex; /* NEW, Spec - Opera 12.1, Firefox 20+ */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer the original here

@metasoarous
Copy link
Member

Thanks so much for submitting this @chena11356!

I'm going to stop reviewing, because it's really challenging to have to wade through all of the whitespace changes. We can discuss a separate issue/PR for this work if you like. Though I have to say, I'm somewhat disinclined from considering something like this across the repo right now with a lot of work going on in various forks that we're trying to get merged. If we do this, we should coordinate with @colinmegill about regarding styleguides and linter settings.

@xeeg xeeg requested review from ballPointPenguin and xeeg April 28, 2023 21:27
@xeeg xeeg added this to the Stable milestone May 1, 2023
@ballPointPenguin
Copy link
Contributor

relates to #217

@ballPointPenguin
Copy link
Contributor

Update: for now we will hide the checkbox for the admin and manually switch it "on" for organizations/conversations as needed.

@ballPointPenguin
Copy link
Contributor

Adding Cypress testing to this feature would be great also

@ballPointPenguin
Copy link
Contributor

closing in favor of #1682

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants