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

Feature/UI enhance ayesha ah2 #195

Closed
wants to merge 6 commits into from
Closed

Conversation

ayeshamk
Copy link
Collaborator

No description provided.

@aih
Copy link
Collaborator

aih commented Feb 26, 2021

The changes in detail.html are still removing many features that were in main. Please copy the entire detail.html from main back into this branch and commit. Then make changes to that version of detail.html by hand (without copy/paste or git merging).

We have two incompatible sets of tab and table html. When these changes are made, please run the app locally and confirm that these tables show data for the appropriate bills: CRS, CBO, related bills, similar bills.

Copy link
Collaborator

@leedavidr leedavidr left a comment

Choose a reason for hiding this comment

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

Hi Ayesha, thank you for the quick fix
I think the fixes caused some more issues though, I reviewed the areas that have issues

I think this is taking Ari a lot of time to merge, and from the last commits, it may seem easier to start from a brand new branch and apply each change with a fresh mind to make sure we don't override anything unintentionally.

I think it will be ok to continue from this branch too, but we need to make sure nothing else is missed via code review, typos & references are corrected (not sure which is correct anymore), and code and functionality is tested locally. It's easier to find the errors by looking at it on the website vs. code. This approach could take more effort.

Let me know when you are ready, maybe you can demo the functionality to me after your changes and I can help test functionally

<hr />
<span>Home > Bills > {{ bill.type }} {{ bill.number }} ({{ bill.congress }}th)</span>
<span><b>Home > Bills > {{ bill.type }} {{ bill.number }} ({{ bill.congress }}th)</b></span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

prefer strong tag over b tag (not needed now but for future reference)
html style tags are considered obsolete, styling is mainly separated to CSS

<h6 class="pt-3">
View all the Statements of Administration Policy for {{ bill.get_type_abbrev }} {{ bill.number }} in the {{ bill.congress }}th Congress
View all the CBO {{ bill.type_abbrev }} {{ bill.meta.number }} in the {{ bill.meta.congres }}th Congress
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this a typo? congres

<h6 class="pt-3">
View all the CBO Reports for {{ bill.get_type_abbrev }} {{ bill.number }} in the {{ bill.congress }}th Congress
View all the Committee Documents {{ bill.type_abbrev }} {{ bill.meta.number }} in the {{ bill.meta.congres }}th Congress
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this a typo? congres

<div class="tab-pane fade" id="statement-admin" role="tabpanel" aria-labelledby="statement-admin-tab" style="min-height:20vh; max-height: 50vh; overflow: scroll;">
<h6 class="pt-3">
View all the Statements of Administration Policy of {{ bill.type_abbrev }} {{ bill.meta.number }} in the {{ bill.meta.congres }}th Congress
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this a typo? congres

one reason to avoid shortening words is to make it easy to read code

Copy link
Collaborator

Choose a reason for hiding this comment

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

This line looks like the error referenced in
#196

Is it because we're using meta.? I see it in other places too, let's make sure we are correctly referencing in all instances

<div class="tab-pane fade show active" id="crs-report" role="tabpanel" aria-labelledby="crs-report-tab">
<h6 class="pt-3">
View CRS reports for {{ bill.get_type_abbrev }} {{ bill.number }} in the {{ bill.congress }}th Congress
View all CRS Reports for {{ bill.type_abbrev }} {{ bill.meta.number }} in the {{ bill.meta.congress }}th Congress
Copy link
Collaborator

Choose a reason for hiding this comment

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

this one has the full congress spelling, I'd make sure it's consistent

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, why is it sometimes bill.meta.number vs. bill.meta.congress? Should it all be the same or is there a difference

<div class="tab-content light-gray-tabs p-4" id="crsReportContent">
<div class="tab-pane fade show active" id="crs-report" role="tabpanel" aria-labelledby="crs-report-tab">
<h6 class="pt-3">
View CRS reports for {{ bill.get_type_abbrev }} {{ bill.number }} in the {{ bill.congress }}th Congress
Copy link
Collaborator

Choose a reason for hiding this comment

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

CRS reports data in the wrong tab? This is related bills tab see comment in issue (#193)

It's better to not rush the fixes, you can make the fix but test it yourself otherwise it adds a lot of time to the cycle. In my experience, fixing bugs often causes more bugs. So it's really important to fully understand the context of the bug before and after, to test properly

@ayeshamk ayeshamk closed this Mar 2, 2021
@aih aih deleted the feature/ui-enhance-ayesha-ah2 branch March 2, 2021 20:08
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.

3 participants