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

interactive: added label to display selected queries to user (5/7) #1773

Merged
merged 41 commits into from
Nov 13, 2023

Conversation

dshaaban01
Copy link
Collaborator

added label to display selected queries to user.

image

xdsl/interactive/app.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 10, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
tests/interactive/test_app.py 100.00% <ø> (ø)
xdsl/interactive/app.py 93.39% <100.00%> (+0.46%) ⬆️

📢 Thoughts on this report? Let us know!

Copy link
Member

@superlopuh superlopuh left a comment

Choose a reason for hiding this comment

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

It feels a little off for the borders to be unaligned above and below, could you please make them line up?

xdsl/interactive/app.tcss Outdated Show resolved Hide resolved
@webmiche
Copy link
Collaborator

I keep getting confused with all of these PRs and how they are ordered. Could you add an index to the PR title? Something like interactive: (3/8) ... 8or interactive: (3/?) ... if you don't know how many there are.

@dshaaban01
Copy link
Collaborator Author

I keep getting confused with all of these PRs and how they are ordered. Could you add an index to the PR title? Something like interactive: (3/8) ... 8or interactive: (3/?) ... if you don't know how many there are.

I originally did that and then Sasha told me it wasn't helpful. Will do this now though.

@webmiche
Copy link
Collaborator

Classic, supervisors giving conflicting advice 😉

@dshaaban01 dshaaban01 changed the title interactive: added label to display selected queries to user interactive: added label to display selected queries to user (5/8) Nov 13, 2023
@dshaaban01 dshaaban01 changed the title interactive: added label to display selected queries to user (5/8) interactive: added label to display selected queries to user (5/7) Nov 13, 2023
@dshaaban01
Copy link
Collaborator Author

Classic, supervisors giving conflicting advice 😉

I've done it now - I think its helpful 😆

@dshaaban01
Copy link
Collaborator Author

@superlopuh Addressed all your comments. But GitHub has blocked on merging, can you unblock me please?

Base automatically changed from dalia/mvp2.2 to main November 13, 2023 13:44
@superlopuh
Copy link
Member

Please merge main and I'll take a look

@dshaaban01
Copy link
Collaborator Author

Please merge main and I'll take a look

main merged :)

Copy link
Member

@superlopuh superlopuh left a comment

Choose a reason for hiding this comment

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

looking good, just want to avoid the same class variable situation with the new setter

@@ -76,6 +79,13 @@ def __init__(self):
self.passes_selection_list = SelectionList(id="passes_selection_list")
super().__init__()

query_label = Label("", id="selected_passes_label")
"""Display's user selected passes"""
Copy link
Member

@superlopuh superlopuh Nov 13, 2023

Choose a reason for hiding this comment

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

Suggested change
"""Display's user selected passes"""
"""Display selected passes."""

query_label = Label("", id="selected_passes_label")
"""Display's user selected passes"""

passes_selection_list: SelectionList[type[ModulePass]] = SelectionList(
Copy link
Member

Choose a reason for hiding this comment

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

please move to init

Copy link
Member

Choose a reason for hiding this comment

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

also please move variable declarations above init

@@ -76,6 +79,13 @@ def __init__(self):
self.passes_selection_list = SelectionList(id="passes_selection_list")
super().__init__()

query_label = Label("", id="selected_passes_label")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
query_label = Label("", id="selected_passes_label")
selected_passes_label = Label("", id="selected_passes_label")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

all comments addressed

xdsl/interactive/app.py Outdated Show resolved Hide resolved
xdsl/interactive/app.py Outdated Show resolved Hide resolved
@dshaaban01 dshaaban01 merged commit b01bb28 into main Nov 13, 2023
10 checks passed
@dshaaban01 dshaaban01 deleted the dalia/mvp2.3 branch November 13, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interactive xdsl-gui things tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants