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

Toolkit scss consolidation #1368

Merged
merged 3 commits into from
Apr 9, 2021

Conversation

sena-ji
Copy link
Member

@sena-ji sena-ji commented Apr 7, 2021

Updated Toolkits pages for standardization

sena-ji added 3 commits April 6, 2021 19:54
- Got rid of .outer-link class
- Added a tag in the toolkit-flex-item.section-container
@akibrhast akibrhast requested review from daniellex0 and jbubar April 7, 2021 03:28
@erikaBell
Copy link
Member

looks good to me

@daniellex0
Copy link
Member

daniellex0 commented Apr 8, 2021

@sena-ji Just confirming, is this connected to an issue? Sorry, I'm not quite sure what the context is! Is this a followup to the Tuesday CSS discussion?

@sena-ji
Copy link
Member Author

sena-ji commented Apr 8, 2021

@sena-ji Just confirming, is this connected to an issue? Sorry, I'm not quite sure what the context is! Is this a followup to the Tuesday CSS discussion?

Nope! I was asked to create a pull request of the standardization we did during the developers meeting.

@daniellex0
Copy link
Member

daniellex0 commented Apr 8, 2021

Ah ok got it- this removes the link from the resource cards? Could you post a screenshot of how it looks with the changes please? I'll run it by the design team meeting tomorrow first if possible since it's a UX design-related change (Also there's a chance Bonnie might want to weigh in on this, I'll check)

@akibrhast
Copy link
Member

Ah ok got it- this removes the link from the resource cards? @daniellex0

No, it does not. Should not.

@daniellex0
Copy link
Member

daniellex0 commented Apr 8, 2021

@akibrhast Ohh ok sorry just ran this myself now and pulled it up (as I should have in the beginning!)- so this should all just look the same, but with more standard CSS, got it. Ok looks good to me.

Just to confirm, .not-clickable is removed because it wasn't being used anywhere, right?

Btw (and this is not a critique at all because I did the same thing forever!), but in the future @sena-ji in PR's please list out the changes that you made with quick explanations if possible, thank you! :) (unless it's already all listed out in an attached issue)

@sena-ji
Copy link
Member Author

sena-ji commented Apr 8, 2021

@akibrhast Ohh ok sorry just ran this myself now and pulled it up (as I should have in the beginning!)- so this should all just look the same, but with more standard CSS, got it. Ok looks good to me.

Just to confirm, .not-clickable is removed because it wasn't being used anywhere, right?

Btw (and this is not a critique at all because I did the same thing forever!), but in the future @sena-ji in PR's please list out the changes that you made with quick explanations if possible, thank you! :) (unless it's already all listed out in an attached issue)

Got it Danielle! I will make sure to do that for future PRs. Thank you for the feedback :)

Copy link
Member

@akibrhast akibrhast left a comment

Choose a reason for hiding this comment

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

LGTM. Am merging

@akibrhast akibrhast merged commit 4110404 into hackforla:gh-pages Apr 9, 2021
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