-
Notifications
You must be signed in to change notification settings - Fork 1
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
Ability to click on the affected URLs of a pattern and see which URLs have been affected #1038
base: dev
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.
Mostly good. I have a few comments. Lmk when you push changes and I can take a look again. Thanks!
sde_collections/views.py
Outdated
def get_queryset(self): | ||
|
||
if "exclude-pattern" in self.request.path: | ||
self.pattern = ExcludePattern.objects.get(id=self.kwargs["id"]) | ||
self.pattern_type = "Exclude" | ||
elif "include-pattern" in self.request.path: | ||
self.pattern = IncludePattern.objects.get(id=self.kwargs["id"]) | ||
self.pattern_type = "Include" | ||
elif "title-pattern" in self.request.path: | ||
self.pattern = TitlePattern.objects.get(id=self.kwargs["id"]) | ||
self.pattern_type = "Title" | ||
elif "document-type-pattern" in self.request.path: | ||
self.pattern = DocumentTypePattern.objects.get(id=self.kwargs["id"]) | ||
self.pattern_type = "Document Type" | ||
else: | ||
return super().get_queryset() | ||
|
||
queryset = self.pattern.matched_urls() | ||
return queryset |
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.
Please create a base class-based view and inherit from it to create multiple views instead of using these if conditions in a single view.
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.
created base-class view which will be inherited by multiple views.
|
||
|
||
|
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.
This file needs linting. VS Code should come with a default linter for CSS files. Maybe you can run "Format files" with Ctrl+Shift+P
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.
solved formatting issue.
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.
These are a lot of additional CSS rules on top of existing ones. Are these all required/used?
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.
Refactored the affected_urls.css to only use required CSS declarations.
}, | ||
|
||
}) |
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.
File needs linting. Please use VS Code's "Format files" feature.
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.
You might consider enabling "Format files on save"
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.
Solved formatting issue.
function handleIncludeIndividualUrlClick() { | ||
$("body").on("click", ".include-url-btn", function () { | ||
|
||
const i = this.querySelector('i'); |
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.
Think this deserves a more descriptive variable name? I'm not sure what i
is from querySelector.
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.
This function is part of the "Include/Exclude URL" feature on the "Affected URLs" page. Currently, the basic version of the "Affected URLs" page has been pushed, which does not incorporate this feature yet. I will address this in my next commit.
render: function (data, type, row) { | ||
return `<div style="display: flex; align-items: center; justify-content: center;"> | ||
<span style="min-width: 50px; text-align: right; padding-right: 10px;">${data}</span> | ||
<button type="button" class="btn btn-sm view-exclude-pattern-urls" data-row-id="${row.id}"> | ||
<i class="fa fa-eye"></i> | ||
</button> | ||
</div>`; | ||
}, | ||
}, |
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.
Let's fix the formatting on these lines
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.
Same comment for all the other render
s we have.
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.
Solved the formatting issue.
function handleShowAffectedURLsListButtonClick() { | ||
$("body").on("click", ".view-exclude-pattern-urls", function () { | ||
var matchPatternId = $(this).data("row-id"); | ||
console.log(matchPatternId); |
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 we need all these console.log
statements in production? If not, consider removing them.
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.
Removed the debugging lines.
<!-- <th scope="col" class="text-center col-3"> | ||
{% if pattern_type == "Include" %} | ||
Exclude URL | ||
{% elif pattern_type == "Exclude" %} | ||
Include URL | ||
{% endif %} | ||
</th> --> |
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.
Let's get rid of commented out code if we don't need it.
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.
Removed the unnecessary comments block.
<!-- <td class="col-3 text-center data-sort="0"> | ||
{% if pattern_type == "Exclude" %} | ||
<a class="include-url-btn" data-url-id="{{ url.id }}" value = "{{url.url}}"> | ||
{% if url.is_included %} | ||
<i class="material-icons tick-mark" style="color: green">check</i></a> | ||
{% else %} | ||
<i class="material-icons cross-mark" style="color: red">close</i></a> | ||
{% endif %} | ||
{% elif pattern_type == "Include" %} | ||
<a class="exclude-url-btn" data-url-id="{{ url.id }}" value = "{{url.url}}"> | ||
{% if url.is_excluded %} | ||
<i class="material-icons tick-mark" style="color: green">check</i></a> | ||
{% else %} | ||
<i class="material-icons cross-mark" style="color: red">close</i></a> | ||
{% endif %} | ||
{% endif %} | ||
</td> --> |
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 we need these? If not, let's remove them.
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.
Removed the unnecessary comments block.
d30541e
to
dad63b0
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Added ability to click on the affected urls of a pattern and see which urls have been affected. This works for Include Patterns, Exclude Patterns, Title Patterns and Document Type Patterns.